Remove tableLayoutHandle #12674

hellium01 · 2019-04-15T21:16:33Z

This PR is to remove tableLayoutHandle completely from PlanNodes and metadata APIs.

For background and future works, see the following comments:

highker · 2019-04-18T02:06:12Z

Is this ready for review?

hellium01 · 2019-04-18T03:53:08Z

I thought it was ok but it seems like there is merge conflict now... Let me rebase it first.

hellium01 · 2019-04-19T16:41:19Z

All the problem fixed, it is now ready to review.

Adding @wenleix /@andrii as it affected TableHandle and getAlternativeTableLayoutHandle, createTemporaryTable.

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

wenleix

"Rename listTableLayouts method" . Looks good (see also trinodb/trino@d7cde8c )
Why rename to pushPredicateIntoTableScan instead of pushFilterIntoTableScan ?
"Add missing null checks on TableLayoutResult constructor" . Looks good. (See also trinodb/trino@c68109c)
"Remove support for multiple layouts" . Looks good. (See also trinodb/trino@801423b).
Please remove unrelated changes in HiveIntegrationSmokeTest.

highker

This is a nice a patch. Reviewed the first 10 commits and skimmed through the rest. There are some api change needs more discussion.

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/AddExchanges.java

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/BeginTableWrite.java

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PickTableLayout.java

presto-main/src/main/java/com/facebook/presto/split/SplitManager.java

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/TableWriterNode.java

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorSplitManager.java

highker · 2019-04-21T05:18:38Z

Might worth splitting everything related to PickTableLayout to a separate PR given @mbasmanova is working on it. There is no logical conflict but better avoid conflicts as early as possible.

wenleix

"Simplify unconditional PickLayout" and "Inline pushPredicateIntoTableScan" Looks good.
"Add tableLayoutHandle and transactionHandle to tableHandle"

Generally looks good. This looks like part of trinodb/trino@47ce7e6 but get split and only adding TableLayoutHandle and TransactionHandle to TableHandle.

Now since we don't remove TableLayoutHandle from TableScanNode and other places, there is an interesting question that for object can both holds TableHandle and TableLayoutHandle, should we check their layout is the same? -- Or maybe we can squash these split commits before merge so this is no longer a problem ? :)

Also, I would use "TableLayoutHandle", "TransactionHandle" and "TableHandle"

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

presto-main/src/main/java/com/facebook/presto/metadata/TableHandle.java

presto-main/src/test/java/com/facebook/presto/cost/TestCostCalculator.java

...ook/presto/sql/planner/iterative/rule/TestTransformCorrelatedSingleRowSubqueryToProject.java

presto-main/src/test/java/com/facebook/presto/sql/planner/iterative/rule/test/PlanBuilder.java

...t/java/com/facebook/presto/sql/planner/sanity/TestValidateAggregationsWithDefaultValues.java

.../src/test/java/com/facebook/presto/sql/planner/sanity/TestValidateStreamingAggregations.java

mbasmanova · 2019-04-22T15:12:05Z

Looks reasonable. I have a question though. pushPredicateIntoTableScan uses Metadata.getLayout API:

Optional<TableLayoutResult> getLayout(Session session, TableHandle tableHandle, Constraint<ColumnHandle> constraint, Optional<Set<ColumnHandle>> desiredColumns);

which uses the following connector API:

    /**
     * Return a list of table layouts that satisfy the given constraint.
     * <p>
     * For each layout, connectors must return an "unenforced constraint" representing the part of the constraint summary that isn't guaranteed by the layout.
     */
    List<ConnectorTableLayoutResult> getTableLayouts(
            ConnectorSession session,
            ConnectorTableHandle table,
            Constraint<ColumnHandle> constraint,
            Optional<Set<ColumnHandle>> desiredColumns);

Is it correct to assume that pushed down predicate is encoded in ConnectorTableLayoutHandle? If so, are you expecting to keep it that way or do you envision changes. If you expected changes, would you describe what they might be?

hellium01 · 2019-04-22T17:53:17Z

We generally should hide ConnectorTableLayoutHandle in ConnectorTableHandle and use ConnectorTableHandle in all metadata API calls. Engine only needs to know that ConnectorTableHandle is updated after the pushdown. It is up to connector to decide what/whether it stores it in a sub data structure or not.

But it is discussible if we want to go that far since what we gain is only code cleanness.

If we allow connector to participate query planning, all these getLayout calls won't be necessary. Connector basically just needs a rule (or a default rule for connectors that does want to implement itself) to take in a subtree + constrained trait set and return a subtree + provided trait set. However, we need to come up with a plan to support both old connector API and new connector rule based planning. Today, we used a lot of specific data structure to bypass the problem connector cannot rewrite query plan (MetadataDelete can be a good example). But it will be a long way to go if we want to move everything over.

wenleix

For now I skip "Use tableHandle in getInfo" to "Remove TableLayoutHandle in TableScanNode" since it looks like continues to backport trinodb/trino@47ce7e6

The following 3 commits looks reasonable:

"Rename TableLayout to TableProperties"
"Rename ConnectorTableLayout to ConnectoLayoutProperties"
"Rename TableLayout to TableProperties in metadata"

Although I feel the third commit should be squashed to the first commit :)

I also agree with @highker 's comment on #12674 (comment) (do we really need to alter the name?) -- or maybe rename it as TableLayoutProperties ?

presto-accumulo/src/main/java/com/facebook/presto/accumulo/AccumuloMetadata.java

presto-main/src/main/java/com/facebook/presto/metadata/TableProperties.java

hellium01

Thanks for reviewing, I will address these comments and as synced offline, will remove the renaming part.

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PickTableLayout.java

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

presto-main/src/test/java/com/facebook/presto/execution/TaskTestUtils.java

...-main/src/test/java/com/facebook/presto/execution/scheduler/TestPhasedExecutionSchedule.java

presto-main/src/test/java/com/facebook/presto/sql/planner/TestTypeValidator.java

presto-main/src/main/java/com/facebook/presto/sql/planner/InputExtractor.java

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorSplitManager.java

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

wenleix

"Use tableHandle in getInfo"

Looks good. Note the semantic is now interesting:

What does TableHandle.getLayout is empty mean?
Should MetadataManager.getLayout always return a layout?

Let's discuss in person and add necessary comments. Otherwise it will be difficult for us to maintain the code (and the open source community :) )

Update:

What does TableHandle.getLayout is empty mean?

Now ......

In the future empty should mean the default access path.

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

wenleix

"Use tableHandle in metadataDelete" Looks good % one nit.

...ain/src/main/java/com/facebook/presto/sql/planner/optimizations/MetadataDeleteOptimizer.java

wenleix

"Use tableHandle in getLayout" . Looks good with a few small questions.

presto-main/src/main/java/com/facebook/presto/metadata/TableHandle.java

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PickTableLayout.java

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

wenleix

(Removed since the comment somehow not published, see next comment instead)

wenleix

"Use tableHandle in getAlternativeTableHandle". Looks good.

presto-main/src/main/java/com/facebook/presto/sql/planner/PlanFragmenter.java

wenleix · 2019-04-25T23:53:34Z

"Remove TableLayoutHandle in TableScanNode" Looks good.
"Use tableHandle in TableProperties". The commit message needs to be updated since we don't have TableProperties anymore :)

wenleix · 2019-04-26T00:24:54Z

Per offline discussion, let's try to squash from "Add tableLayoutHandle and transactionHandle to tableHandle" to "Remove TableLayoutHandle in TableScanNode". Remember to have a backup before squash :)

hellium01

Thanks for reviewing, comments has been addressed.

presto-main/src/main/java/com/facebook/presto/split/SplitManager.java

...o-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/ExtractSpatialJoins.java

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

...ain/src/main/java/com/facebook/presto/sql/planner/optimizations/MetadataDeleteOptimizer.java

hellium01 · 2019-04-26T19:24:48Z

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

@@ -74,7 +74,7 @@

    Optional<TableHandle> getTableHandleForStatisticsCollection(Session session, QualifiedObjectName tableName, Map<String, Object> analyzeProperties);

-    List<TableLayoutResult> getLayouts(Session session, TableHandle tableHandle, Constraint<ColumnHandle> constraint, Optional<Set<ColumnHandle>> desiredColumns);
+    Optional<TableLayoutResult> getLayout(Session session, TableHandle tableHandle, Constraint<ColumnHandle> constraint, Optional<Set<ColumnHandle>> desiredColumns);


Updated in previous commit ("Remove support for multiple layouts").

presto-main/src/main/java/com/facebook/presto/metadata/TableHandle.java

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PickTableLayout.java

presto-main/src/main/java/com/facebook/presto/sql/planner/PlanFragmenter.java

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/TableScanNode.java

wenleix

Some nits. Will continue review.

presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/AddExchanges.java

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/BeginTableWrite.java

presto-main/src/main/java/com/facebook/presto/metadata/TableHandle.java

wenleix

Looks great. All comments are minor. Thanks for clean it up!

Let's quickly go through the comments marked with "talk in person".

Minor comments about commit message:

"Hide TableLayout from engine"
Remember to wrap the commit message to 72 characters :)
"Use TableHandle in TableLayout"
Technically TableHandle is not used in TableLayout -- TableLayout now contains the information to construct a TableHandle, but it doesn't store TableHandle as a field.
So what about "Remove TableLayoutHandle from TableLayout" ?

presto-main/src/main/java/com/facebook/presto/sql/planner/PlanFragmenter.java

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PickTableLayout.java

wenleix · 2019-04-27T19:54:56Z

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PickTableLayout.java

                session,
                node.getTable(),
                constraint,
                Optional.of(node.getOutputSymbols().stream()
                        .map(node.getAssignments()::get)
                        .collect(toImmutableSet())));

-        if (layouts.isEmpty()) {
-            return ImmutableList.of(new ValuesNode(idAllocator.getNextId(), node.getOutputSymbols(), ImmutableList.of()));
+        if (layout.getLayout().getPredicate().isNone()) {


I am a bit confused about the logic from line 301-303, although I understand the semantic is correct since it's doing the same thing as before. Let's quickly discuss in person :)

Looks like TableLayout. getPredicate.isNone() somewhat indicates the scan result is empty? (So use an empty ValuesNode to replace TableScanNode )

Yes, before we even pushdown, we will eliminate unnecessary table Scan node. An example is if we are reading from a view where intersect of predicate from the view and from the query will yield empty result.

wenleix · 2019-04-27T19:57:24Z

...ain/src/main/java/com/facebook/presto/sql/planner/optimizations/MetadataDeleteOptimizer.java

                return context.defaultRewrite(node);
            }
-            return new MetadataDeleteNode(idAllocator.getNextId(), delete.get().getTarget(), Iterables.getOnlyElement(node.getOutputSymbols()), tableScanNode.getLayout().get());
+            // delete target is always the table in source tableScanNode, see BeginTableWrite for details.


I thought i understand the code but I don't quite understand this comment. Let's quickly discuss in person :)

More accurately, constructs a set of alternate plans with the filter pushed into the table scan based on available table layouts.

Currently multiple layout is used for streaming aggregation/pre-sorted layout selection for connector that supported it. We should have better way to support this functionality once we start to allow connector participate planning. We now expects connector to return at least one layout.

It's just selecting a layout for the raw table scan, so no need to go through the logic for pushing a predicate etc.

This commit hide tableLayout from engine. Since connector will rewrite the sub plan and use an new TableHandle to represent the current provided view of the data set, layout is no longer a useful concept.

facebook-github-bot added the CLA Signed label Apr 15, 2019

hellium01 force-pushed the RefactorPickLayout branch 8 times, most recently from 945d64f to 9c9a07d Compare April 19, 2019 08:28

hellium01 requested review from highker and wenleix April 19, 2019 16:37

hellium01 requested a review from arhimondr April 19, 2019 16:41

wenleix reviewed Apr 19, 2019

View reviewed changes

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java Outdated Show resolved Hide resolved

wenleix reviewed Apr 20, 2019

View reviewed changes

wenleix self-assigned this Apr 20, 2019

highker reviewed Apr 21, 2019

View reviewed changes

wenleix reviewed Apr 21, 2019

View reviewed changes

wenleix reviewed Apr 22, 2019

View reviewed changes

presto-accumulo/src/main/java/com/facebook/presto/accumulo/AccumuloMetadata.java Outdated Show resolved Hide resolved

wenleix reviewed Apr 22, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/metadata/TableProperties.java Outdated Show resolved Hide resolved

hellium01 commented Apr 25, 2019

View reviewed changes

wenleix reviewed Apr 25, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java Outdated Show resolved Hide resolved

hellium01 force-pushed the RefactorPickLayout branch 2 times, most recently from d990f5e to 4e1a6c2 Compare April 25, 2019 09:22

wenleix reviewed Apr 25, 2019

View reviewed changes

...ain/src/main/java/com/facebook/presto/sql/planner/optimizations/MetadataDeleteOptimizer.java Outdated Show resolved Hide resolved

wenleix reviewed Apr 25, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/planner/PlanFragmenter.java Outdated Show resolved Hide resolved

hellium01 commented Apr 26, 2019

View reviewed changes

hellium01 force-pushed the RefactorPickLayout branch 3 times, most recently from 210128f to fc1261b Compare April 27, 2019 00:49

wenleix reviewed Apr 27, 2019

View reviewed changes

wenleix approved these changes Apr 27, 2019

View reviewed changes

wenleix assigned hellium01 and unassigned wenleix Apr 27, 2019

hellium01 force-pushed the RefactorPickLayout branch from fc1261b to 8cad186 Compare April 29, 2019 18:17

martint and others added 5 commits April 29, 2019 13:23

Rename listTableLayouts method

1752ffa

More accurately, constructs a set of alternate plans with the filter pushed into the table scan based on available table layouts.

Add missing null checks on TableLayoutResult constructor

8bda792

Simplify unconditional PickLayout

3503ad5

It's just selecting a layout for the raw table scan, so no need to go through the logic for pushing a predicate etc.

Inline pushPredicateIntoTableScan

e48e567

hellium01 force-pushed the RefactorPickLayout branch 2 times, most recently from 5d4b78f to 2f22bff Compare April 29, 2019 21:22

hellium01 added 3 commits April 29, 2019 17:00

Hide TableLayout from engine

c82fa39

This commit hide tableLayout from engine. Since connector will rewrite the sub plan and use an new TableHandle to represent the current provided view of the data set, layout is no longer a useful concept.

Remove TableLayoutHandle from TableLayout

d376d83

Remove TableLayoutHandle completely

58ac906

hellium01 force-pushed the RefactorPickLayout branch from 2f22bff to 58ac906 Compare April 30, 2019 00:00

hellium01 merged commit 4c07f12 into prestodb:master Apr 30, 2019

16pierre mentioned this pull request Nov 10, 2023

Avoid exchange for Connectors that support multiple layouts #21366

Open

hantangwangd mentioned this pull request Jan 24, 2024

Refactor ConnectorMetadata to deprecate multi-layouts returning method #21767

Closed

6 tasks

hantangwangd mentioned this pull request Feb 14, 2024

Refactor ConnectorMetadata to deprecate multi-layouts returning method #21933

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove tableLayoutHandle #12674

Remove tableLayoutHandle #12674

hellium01 commented Apr 15, 2019 •

edited by wenleix

Loading

highker commented Apr 18, 2019

hellium01 commented Apr 18, 2019

hellium01 commented Apr 19, 2019 •

edited

Loading

wenleix left a comment •

edited

Loading

highker left a comment

highker commented Apr 21, 2019

wenleix left a comment

mbasmanova commented Apr 22, 2019

hellium01 commented Apr 22, 2019 •

edited by wenleix

Loading

wenleix left a comment •

edited

Loading

hellium01 left a comment

wenleix left a comment •

edited

Loading

wenleix left a comment

wenleix left a comment

wenleix left a comment •

edited

Loading

wenleix left a comment

wenleix commented Apr 25, 2019

wenleix commented Apr 26, 2019

hellium01 left a comment

hellium01 Apr 26, 2019

wenleix left a comment

wenleix left a comment

wenleix Apr 27, 2019

hellium01 Apr 29, 2019

wenleix Apr 27, 2019

Remove tableLayoutHandle #12674

Remove tableLayoutHandle #12674

Conversation

hellium01 commented Apr 15, 2019 • edited by wenleix Loading

highker commented Apr 18, 2019

hellium01 commented Apr 18, 2019

hellium01 commented Apr 19, 2019 • edited Loading

wenleix left a comment • edited Loading

Choose a reason for hiding this comment

highker left a comment

Choose a reason for hiding this comment

highker commented Apr 21, 2019

wenleix left a comment

Choose a reason for hiding this comment

mbasmanova commented Apr 22, 2019

hellium01 commented Apr 22, 2019 • edited by wenleix Loading

wenleix left a comment • edited Loading

Choose a reason for hiding this comment

hellium01 left a comment

Choose a reason for hiding this comment

wenleix left a comment • edited Loading

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

wenleix left a comment • edited Loading

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

wenleix commented Apr 25, 2019

wenleix commented Apr 26, 2019

hellium01 left a comment

Choose a reason for hiding this comment

hellium01 Apr 26, 2019

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

wenleix Apr 27, 2019

Choose a reason for hiding this comment

hellium01 Apr 29, 2019

Choose a reason for hiding this comment

wenleix Apr 27, 2019

Choose a reason for hiding this comment

hellium01 commented Apr 15, 2019 •

edited by wenleix

Loading

hellium01 commented Apr 19, 2019 •

edited

Loading

wenleix left a comment •

edited

Loading

hellium01 commented Apr 22, 2019 •

edited by wenleix

Loading

wenleix left a comment •

edited

Loading

wenleix left a comment •

edited

Loading

wenleix left a comment •

edited

Loading