Cleanup metadata manager by dain · Pull Request #10091 · trinodb/trino

dain · 2021-11-28T21:38:20Z

Metadata is being used as a dumping ground for unrelated services, because it is present in all places in the analyzer and optimizer. This PR makes more parts of the analyzer injectable, and remove most unnecessary services from metadata manager.

martint

Reviewed up to "Remove SessionPropertyManager from Metadata"

plugin/trino-thrift/src/test/java/io/trino/plugin/thrift/integration/ThriftQueryRunner.java

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzerFactory.java

core/trino-main/src/main/java/io/trino/connector/ConnectorManager.java

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

core/trino-main/src/test/java/io/trino/execution/TestCreateMaterializedViewTask.java

core/trino-main/src/main/java/io/trino/metadata/BlockEncodingManager.java

core/trino-main/src/main/java/io/trino/metadata/LiteralFunction.java

core/trino-main/src/main/java/io/trino/metadata/TypeRegistry.java

electrum · 2021-12-19T18:28:38Z

core/trino-main/src/main/java/io/trino/metadata/TypeRegistry.java

You could convert this to LoadingCache now

core/trino-main/src/main/java/io/trino/execution/SetSessionTask.java

core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java

electrum · 2021-12-19T23:01:34Z

core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java

Does it make sense to extract Metadata in the constructor for all of these, since that's the only field they use?

All of these tasks actually use the planner context. The downstream services need it.

findepi · 2021-12-21T09:41:38Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

                .addAll(new UnwrapRowSubscript().rules())
                .addAll(new PushCastIntoRow().rules())
-                .addAll(new UnwrapCastInComparison(metadata, typeOperators, typeAnalyzer).rules())
+                .addAll(new UnwrapCastInComparison(plannerContext, typeAnalyzer).rules())


any reason why planner context doesn't include the type analyzer?

findepi · 2021-12-21T09:45:31Z

core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java

                                new RemoveRedundantIdentityProjections(),
                                new PushDownProjectionsFromPatternRecognition())),
-                new MetadataQueryOptimizer(metadata),
+                new MetadataQueryOptimizer(plannerContext),


The choice which optimizers now take plannerContext and which continue to take metadata looks arbitrary. What's the rule?

findepi · 2021-12-21T10:11:07Z

core/trino-main/src/main/java/io/trino/sql/planner/DomainTranslator.java

-            Session session,
-            Expression predicate,
-            TypeProvider types)
+    public static ExtractionResult getExtractionResult(PlannerContext plannerContext, Session session, Expression predicate, TypeProvider types)


Update TestDomainTranslator.testFrom*Predicate* methods

findepi · 2021-12-21T10:30:22Z

core/trino-main/src/main/java/io/trino/sql/planner/TypeAnalyzer.java


    @Inject
-    public TypeAnalyzer(SqlParser parser, Metadata metadata)
+    public TypeAnalyzer(PlannerContext plannerContext, StatementAnalyzerFactory statementAnalyzerFactory, AccessControl accessControl)


Previous TypeAnalyzer signature was: TypeAnalyzer(SqlParser parser, Metadata metadata).
That was kind of easy to understand.
Why does it need all these things now?
@martint can you perhaps explain this to me?

In particular, in a unit test i could create TypeAnalyzer like this

TypeAnalyzer typeAnalyzer = new TypeAnalyzer(new SqlParser(), createTestMetadataManager());

Now it seems this takes

Metadata metadata = createTestMetadataManager(); TypeOperators typeOperators = new TypeOperators(); InternalTypeManager typeManager = new InternalTypeManager(new TypeRegistry(typeOperators, new FeaturesConfig())); PlannerContext plannerContext = new PlannerContext( metadata, typeOperators, new InternalBlockEncodingSerde(new BlockEncodingManager(), typeManager), TESTING_TYPE_MANAGER); TypeAnalyzer typeAnalyzer = createTestingTypeAnalyzer(plannerContext);

to get same thing.

dain requested review from electrum and martint November 28, 2021 21:38

cla-bot bot added the cla-signed label Nov 28, 2021

dain force-pushed the cleanup-metadata-manager branch from 01dfcf8 to 81b718c Compare December 4, 2021 20:51

dain mentioned this pull request Dec 4, 2021

Aggregation and window refactor part 1 #10183

Merged

dain force-pushed the cleanup-metadata-manager branch 3 times, most recently from c307fab to a9d2a16 Compare December 5, 2021 22:06

dain force-pushed the cleanup-metadata-manager branch 4 times, most recently from 7fa8be0 to 75c801b Compare December 18, 2021 03:54

martint reviewed Dec 18, 2021

View reviewed changes

dain force-pushed the cleanup-metadata-manager branch 3 times, most recently from 2987b03 to 4b6fdd2 Compare December 18, 2021 20:52

electrum approved these changes Dec 19, 2021

View reviewed changes

dain added 13 commits December 19, 2021 15:56

Simplify PlanOptimizersProvider interface in LocalQueryRunner

679539e

Cleanup explain query testing infrastructure

19a4bff

Simplify SetTimeZoneTask

eb92fb7

Remove unused getAnalyzerFactory methods

6faf6ec

Simplify statement and expression analyzer creation

1cf6ec8

Remove ProcedureRegistry from Metadata

e122d9e

Remove TableProceduresRegistry from Metadata

f9416dd

Remove SessionPropertyManager from Metadata

04af2d5

Remove SchemaPropertyManager from Metadata

28fc2c1

Remove ColumnPropertyManager from Metadata

fdfdbf4

Remove TablePropertyManager from Metadata

edb2ee0

Remove MaterializedViewPropertyManager from Metadata

62980eb

Remove AnalyzePropertyManager from Metadata

f114401

dain added 9 commits December 19, 2021 16:12

Remove TableProceduresPropertyManager from Metadata

f11097c

Remove block encoding management from MetadataManager

fba1b54

Remove type registration from Metadata

b674fa3

Move session to from a field to a parameter in LiteralEncoder

a1d79a8

Add PlannerContext

e9a986d

Remove BlockEncodingSerde from metadata

1bbf601

Replace most uses of TypeRegistry with TypeManager

0331244

Replace Metadata type methods with TypeManager

0fa7650

Remove createExpressionAnalyzer from StatementAnalyzerFactory

17fa973

dain force-pushed the cleanup-metadata-manager branch from 4b6fdd2 to 17fa973 Compare December 20, 2021 00:47

dain merged commit c13f952 into trinodb:master Dec 20, 2021

dain deleted the cleanup-metadata-manager branch December 20, 2021 02:13

github-actions bot added this to the 367 milestone Dec 20, 2021

findepi reviewed Dec 21, 2021

View reviewed changes

findepi mentioned this pull request Dec 21, 2021

Avoid calling access control during the planning phase #10369

Closed

findepi mentioned this pull request Jan 19, 2022

Optimize effectively literal values #10663

Merged

Conversation

dain commented Nov 28, 2021

Uh oh!

martint left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants