Pushdown subscript paths into project on top of scan #12571

mbasmanova · 2019-04-01T21:13:57Z

No description provided.

elonazoulay

LGTM!

tdcmeehan

I took an initial pass. I have a question about the intent behind the SPI change.

tdcmeehan · 2019-04-04T06:26:02Z

presto-spi/src/main/java/com/facebook/presto/spi/SubfieldPathTokenizer.java

+import static java.lang.String.format;
+import static java.util.Objects.requireNonNull;
+
+public class SubfieldPathTokenizer


Correct me if I'm wrong but I see some inspiration from AbstractIterator :) If so I'd leave a comment explaining. Or perhaps for the benefit of others using SPI we copy it over?

@tdcmeehan You are correct :-) Originally, I copied AbstractIterator verbatim, but it pulls in some unnecessary dependencies and code style in that class doesn't match Presto. Hence, I folded the logic into SubfieldPathTokenizer.

tdcmeehan · 2019-04-04T06:33:09Z

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

+
+            ProjectNode projectNode = new ProjectNode(
+                    idAllocator.getNextId(),
+                    new TableScanNode(


I came across a similar thing and thought it might be better to flatten. While having this nested with indentation shows the layout, it can make it hard to see which parameter belongs to which node. This is just my personal preference though.

@tdcmeehan How about this?

TableScanNode tableScanNode = new TableScanNode( node.getId(), node.getTable(), node.getOutputSymbols().stream() .map(s -> symbolMap.getOrDefault(s, s)) .collect(toImmutableList()), newAssignments.build(), node.getLayout(), node.getCurrentConstraint(), node.getEnforcedConstraint()); return new ProjectNode(idAllocator.getNextId(), tableScanNode, projections.build());

tdcmeehan · 2019-04-04T06:53:13Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

@@ -1652,6 +1652,33 @@ public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHa
        return true;
    }

+    @Override
+    public Map<ColumnHandle, ColumnHandle> pushdownSubfieldPruning(


Is this for prototyping or do we envision this being the standard for master?

@tdcmeehan It depends on whether @highker's work on allowing connectors to participate in query planning lands soon enough. Ideally, we would present project(filter(scan)) subplan to the connector and it will decide that it can implement all of it and return a new table scan node with table and column handles encoding projections and filters. If this can't happen soon enough, I do envision introducing a couple of new metadata APIs to allow for pushing down some information into the connector. Specifically,

pushdownSubfieldPruning tells the connector that it can prune structs, maps and arrays

getTableLayouts(TupleDomain<ColumnHandleWithSubfieldPaths>) allows the connector to implement simple filters on subfields, e.g. c[2] = 5.

Sounds good. I would add a comment discouraging its use before it reaches master.

tdcmeehan

% nits

tdcmeehan · 2019-04-08T16:04:09Z

presto-tpch/src/main/java/com/facebook/presto/tpch/TpchMetadata.java

+            Map<ColumnHandle, List<SubfieldPath>> desiredSubfields)
+    {
+        if (!isSubfieldPushdownEnabled(session)) {
+            return Collections.emptyMap();


ImmutableMap.of()

tdcmeehan · 2019-04-08T16:05:48Z

presto-tpch/src/main/java/com/facebook/presto/tpch/TpchMetadata.java

@@ -119,6 +127,18 @@
            .map(value -> new NullableValue(getPrestoType(PartColumn.CONTAINER), value))
            .collect(toSet());

+    private static final Map<String, List<ColumnMetadata>> EXTRA_TABLES = ImmutableMap.<String, List<ColumnMetadata>>builder()
+                .put(
+                        "lineitem_ext",


I would put a comment here. We're going to remove it before master right?

@tdcmeehan We'll need something like this to test on master as well. One option would be to add a new TpchPlusConnector and leave TpchConnector being purely TPC-H. Another option would be to allow extended TPC-H connector like this.

tdcmeehan · 2019-04-08T16:06:28Z

presto-tpch/src/main/java/com/facebook/presto/tpch/TpchSessionProperties.java

+    private static final List<PropertyMetadata<?>> sessionProperties = ImmutableList.of(
+            booleanProperty(
+                    PUSHDOWN_SUBFIELDS,
+                    "Enable bucket-aware execution: only use a single worker per bucket",


Seems incorrect

tdcmeehan · 2019-04-08T16:08:04Z

presto-tpch/src/main/java/com/facebook/presto/tpch/TpchColumnHandle.java

@@ -27,14 +30,22 @@
 {
    private final String columnName;
    private final Type type;
+    private final Optional<List<SubfieldPath>> subfieldPaths;


Can we represent emptiness with the list? Empty list rather than Optional.empty

tdcmeehan · 2019-04-08T16:15:44Z

presto-spi/src/main/java/com/facebook/presto/spi/SubfieldPath.java

+    {
+        SubfieldPathTokenizer tokenizer = new SubfieldPathTokenizer(path);
+        List<PathElement> elements = new ArrayList<>();
+        while (tokenizer.hasNext()) {


Suggested change

while (tokenizer.hasNext()) {

tokenizer.forEachRemaining(elements::add);

tdcmeehan · 2019-04-08T16:16:45Z

presto-spi/src/main/java/com/facebook/presto/spi/SubfieldPath.java

-            @JsonSubTypes.Type(value = LongSubscript.class, name = "longSubscript"),
-            @JsonSubTypes.Type(value = StringSubscript.class, name = "stringSubscript"),
-            @JsonSubTypes.Type(value = AllSubscripts.class, name = "allSubscripts"),
-    })
    public abstract static class PathElement


Nice, I like this change

tdcmeehan · 2019-04-08T16:17:47Z

presto-main/src/test/java/com/facebook/presto/metadata/AbstractMockMetadata.java

+    @Override
+    public Map<ColumnHandle, ColumnHandle> pushdownSubfieldPruning(Session session, TableHandle tableHandle, Map<ColumnHandle, List<SubfieldPath>> desiredSubfields)
+    {
+        throw new UnsupportedOperationException();


Any harm in returning empty map?

@tdcmeehan All other methods here throw, hence, I did the same.

tdcmeehan · 2019-04-08T16:22:37Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

@@ -1652,6 +1652,33 @@ public boolean supportsMetadataDelete(ConnectorSession session, ConnectorTableHa
        return true;
    }

+    @Override
+    public Map<ColumnHandle, ColumnHandle> pushdownSubfieldPruning(


Sounds good. I would add a comment discouraging its use before it reaches master.

mbasmanova · 2019-04-08T17:32:20Z

@tdcmeehan Tim, thank you for review. Comments addressed.

Rename PushdownSubscripts rule into PushdownSubfields. Change its logic to create project node on top of scan using new filter_by_subfield_paths function instead of modifying column handles in table scan node via calls to ColumnHandle.createSubfieldPruningColumnHandle. Remove ColumnHandle.createSubfieldPruningColumnHandle API.

Add an optimizer rule to pushdown subfield pruning into connectors. The new rule uses the new metadata API pushdownSubfieldPruning.

mbasmanova requested review from elonazoulay, tdcmeehan, yingsu00 and oerling April 1, 2019 21:13

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

mbasmanova mentioned this pull request Apr 2, 2019

Initial refactoring of pushdown subscripts rule #12534

Merged

elonazoulay approved these changes Apr 2, 2019

View reviewed changes

mbasmanova force-pushed the pushdown-subscript-paths branch 6 times, most recently from 306ff04 to 4e56af1 Compare April 3, 2019 19:03

mbasmanova added 3 commits April 3, 2019 15:04

Add filter_by_subscript_paths function

47b34ab

Add pushdown_subfields session property

0fa3770

Rename SubfieldPath.getPath to getPathElements

da86c0b

mbasmanova force-pushed the pushdown-subscript-paths branch from 4e56af1 to 8ae386d Compare April 3, 2019 19:04

tdcmeehan reviewed Apr 4, 2019

View reviewed changes

highker added the aria Presto Aria performance improvements label Apr 4, 2019

mbasmanova force-pushed the pushdown-subscript-paths branch from 8ae386d to f9379c7 Compare April 4, 2019 20:50

tdcmeehan approved these changes Apr 8, 2019

View reviewed changes

Serialize SubfieldPath into a string

dde9a8e

mbasmanova force-pushed the pushdown-subscript-paths branch from f9379c7 to 75dc441 Compare April 8, 2019 17:31

tdcmeehan approved these changes Apr 8, 2019

View reviewed changes

mbasmanova added 3 commits April 8, 2019 20:00

Remove ColumnHandle.getReferencedSubfields API

db7faf4

Add PushdownSubfieldsIntoConnector rule

2de138c

Add an optimizer rule to pushdown subfield pruning into connectors. The new rule uses the new metadata API pushdownSubfieldPruning.

mbasmanova force-pushed the pushdown-subscript-paths branch from 75dc441 to 2de138c Compare April 9, 2019 00:00

mbasmanova merged this pull request into prestodb:aria-scan-research Apr 9, 2019

mbasmanova deleted the pushdown-subscript-paths branch May 24, 2019 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pushdown subscript paths into project on top of scan #12571

Pushdown subscript paths into project on top of scan #12571

mbasmanova commented Apr 1, 2019

elonazoulay left a comment

tdcmeehan left a comment

tdcmeehan Apr 4, 2019

mbasmanova Apr 4, 2019

tdcmeehan Apr 4, 2019

mbasmanova Apr 4, 2019

tdcmeehan Apr 4, 2019

mbasmanova Apr 4, 2019

tdcmeehan Apr 8, 2019

tdcmeehan left a comment

tdcmeehan Apr 8, 2019

tdcmeehan Apr 8, 2019

mbasmanova Apr 8, 2019

tdcmeehan Apr 8, 2019

tdcmeehan Apr 8, 2019

tdcmeehan Apr 8, 2019

tdcmeehan Apr 8, 2019

tdcmeehan Apr 8, 2019

mbasmanova Apr 8, 2019

tdcmeehan Apr 8, 2019

mbasmanova commented Apr 8, 2019

	while (tokenizer.hasNext()) {
	tokenizer.forEachRemaining(elements::add);

Pushdown subscript paths into project on top of scan #12571

Pushdown subscript paths into project on top of scan #12571

Conversation

mbasmanova commented Apr 1, 2019

elonazoulay left a comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova commented Apr 8, 2019