Initial refactoring of pushdown subscripts rule #12534

mbasmanova · 2019-03-26T22:40:52Z

Extracted pushdown subscripts logic into its own rule, PushDownSubscripts,
from PruneUnreferencedOutputs.
Replaced Subfield.PathElement with a hierarchy of classes: empty base class
PathElement + NestedField, IntegerSubscript, StringSubscript sub-classes.
NestedField represents subfield of a struct: c.a, IntegerSubscript - element
of an array or map with integer keys: c[2], StringSubscript - element of a
map with varchar keys c["a"].

Supersedes #12515.

mbasmanova · 2019-03-28T14:48:01Z

@oerling Addressed offline comments from Orri.

tdcmeehan · 2019-04-02T05:06:45Z

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

    }

    @Override
    public int hashCode()
    {
-        return path.hashCode();
+        return Objects.hash(path, getName());


Because getName always returns path.toString, do we really need it? Similar question for equals? If so can we comment why?

@tdcmeehan You are correct. I got confused with this class. It extends Symbol, but it doesn't work like Symbol at all. For example, toSymbolReference will return an invalid symbol with empty name... Let me revert changes to hashCode and equals. In some followup PR, I hope to remove this class altogether.

tdcmeehan · 2019-04-02T05:32:33Z

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

    {
        requireNonNull(path, "path is null");
+        if (path.size() <= 1) {


I would create a private static checkArgument method for these checks

tdcmeehan · 2019-04-02T05:51:31Z

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


+    public static PathElement allSubscripts()


It's not clear to me what this means, can you add a comment for it?

@tdcmeehan Let's say you have column coordinates of type array(row(x, y)) and the query is:

SELECT c.x FROM t CROSS JOIN UNNEST(coordinates) as u(c)

By default, table scan will produce coordinates column where each array element contains both x and y. However, the query only uses x. To express that, you'll use SubscriptPath coordinates[*].x. allSubscripts is used to express *.

I can't come up with a good comment to put here. Any ideas? I'd like to avoid restating the obvious, e.g. "Represents all subscripts of an array or map."

What do you think of expressing all subscripts as its own subclass of PathElement, rather than overloading LongSubscript with the magic number? It could be singleton.

@tdcmeehan I think is it better. We can then serialize it as "*" which will read better. Would you like me to make this change in this PR?

tdcmeehan · 2019-04-02T06:32:29Z

presto-orc/src/main/java/com/facebook/presto/orc/reader/StructStreamReader.java

@@ -93,28 +95,24 @@
    @Override
    public void setReferencedSubfields(List<SubfieldPath> subfields, int depth)
    {
-        HashMap<String, ArrayList<SubfieldPath>> subfieldPaths = new HashMap();
+        Map<String, List<SubfieldPath>> subfieldPaths = new HashMap();


Can we rename this so that the key is included in the name? i.e. what is the String?

tdcmeehan · 2019-04-02T06:36:52Z

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

-        }
-        else {
-            return assignments.get(symbol);
+            SubfieldPath path = ((SymbolWithSubfieldPath) symbol).getPath();


I would rename getPath to getSubfieldPath for clarity

tdcmeehan · 2019-04-02T06:37:30Z

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

+
+        private static boolean prefixExists(SubfieldPath subfieldPath, Collection<SubfieldPath> subfieldPaths)
+        {
+            List<PathElement> path = subfieldPath.getPath();


I would rename getPath here to something like getPathParts

@tdcmeehan I did that in #12571 (getPathElements)

tdcmeehan · 2019-04-02T06:42:21Z

presto-orc/src/main/java/com/facebook/presto/orc/reader/ListStreamReader.java

@@ -73,7 +75,7 @@
    // The set of subscripts for which data needs to be
    // returned. Other positions can be initialized to null. If this
    // is null, values for all subscripts must be returned.
-    long[] subscripts;
+    private int[] subscripts;


Is it because we can never have such a ridiculously high subscript? If so, why are we using LongSubscript instead of for example IntegerSubscript?

@tdcmeehan LongSubscript is used for both array subscript (where values can only be integers) and map subscript (where values can be both integers and longs, e.g. map<integer, T> and map<bigint, T>).

Do you think it makes sense to differentiate it when we parse? From my reading we should be able to tell the difference when we construct the XxSubscript (correct me if I'm wrong?)

@tdcmeehan At this point, I don't see any practical value of doing that. It would require examining the type of the SubscriptExpression#base, which is not readily available.

tdcmeehan · 2019-04-02T06:45:43Z

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

-                else if (field != null) {
-                    base = new DereferenceExpression(base, new Identifier(field));
+                else if (element instanceof LongSubscript) {
+                    base = new SubscriptExpression(base, new LongLiteral(String.valueOf(((LongSubscript) element).getIndex())));


I think there's some extra parenthesis here and below

@tdcmeehan I don't see that. Would you show a rewrite?

You're right, I didn't read it correctly.

mbasmanova · 2019-04-02T15:31:24Z

@tdcmeehan Tim, thank you for review. Comments addressed. Would you take another look?

tdcmeehan · 2019-04-02T18:24:57Z

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

+                        elements.add(new SubfieldPath.LongSubscript(Long.valueOf(literal.getValue())));
+                    }
+                    else {
+                        return null;


Could this cause NPE on any of deferenceOrSubscriptExpressionToPath's callsites?

@tdcmeehan It might. Some callers check for null and some don't. This is an existing behavior, hence, I'd like to revisit it in some future PR.

tdcmeehan · 2019-04-02T18:45:24Z

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

+        private static boolean prefixExists(SubfieldPath subfieldPath, Collection<SubfieldPath> subfieldPaths)
+        {
+            List<PathElement> path = subfieldPath.getPath();
+            for (SubfieldPath otherSubfiledPath : subfieldPaths) {


I would consider adding a isPrefix method to SubfieldPath. (Previously I mentioned something comparable etc, don't think it's necessary.)

tdcmeehan · 2019-04-02T18:54:29Z

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

+                // Compute the leaf subfields. If we have a.b.c and
+                // a.b then a.b is the result. If a path is a prefix
+                // of another path, then the longer is discarded.
+                List<SubfieldPath> leafPaths = new ArrayList();


I've read it a couple of times now, and I believe this comment isn't accurate. I think it actually returns a.b.c, not a.b.

@tdcmeehan "a.b".isPrefix("a.b.c") returns true, hence, "a.b" will be added to leafPaths, but "a.b.c" won't.

mbasmanova · 2019-04-02T19:48:04Z

@tdcmeehan Tim, thank you for review. I added isPrefix method to SubfieldPath and AllSubscripts class. Would you take another look?

tdcmeehan · 2019-04-02T20:06:59Z

presto-orc/src/main/java/com/facebook/presto/orc/reader/ListStreamReader.java

-            }
-            if (subscript.getSubscript() == PathElement.allSubscripts) {
+            checkArgument(subscript instanceof LongSubscript, "List reader needs a PathElement with a subscript");
+            if (subscript == allSubscripts()) {


Because it can be serialized/deserialized, is it safe to use reference equality?

Even if practically it will never happen (not sure), we could ensure it by using @JsonCreator on a static factory which always returns the static constant, and making the constructor private.

@tdcmeehan Good point. It is not safe as of right now, but it will be safe once we use custom deserialization from #12571 (it won't be calling new AllSubscripts(), but use allSubscripts() factory method.) That said, I could change this to .equals() and implement AllSubscripts.equals(). What do you think?

tdcmeehan

Besides the improvement suggestion here:

#12534 (comment)

LGTM

- Extracted pushdown subscripts logic into its own rule, PushDownSubscripts, from PruneUnreferencedOutputs. - Replaced Subfield.PathElement with a hierarchy of classes: empty base class PathElement + NestedField, IntegerSubscript, StringSubscript sub-classes. NestedField represents subfield of a struct: c.a, IntegerSubscript - element of an array or map with integer keys: c[2], StringSubscript - element of a map with varchar keys c["a"].

mbasmanova · 2019-04-03T00:25:55Z

@tdcmeehan Thank you, Tim. I updated AllSubscripts with @JsonCreator on a static factory.

mbasmanova requested review from elonazoulay, tdcmeehan, yingsu00 and oerling March 26, 2019 22:40

facebook-github-bot added the CLA Signed label Mar 26, 2019

mbasmanova mentioned this pull request Mar 26, 2019

[WIP] Initial refactoring of pushdown subscripts rule #12515

Closed

mbasmanova force-pushed the refactor-aria-rules branch from c9953e4 to b5207bc Compare March 28, 2019 14:45

wenleix assigned mbasmanova Mar 28, 2019

phd3 mentioned this pull request Mar 29, 2019

Pushing down reducing projections in a more generic manner trinodb/trino#567

Open

tdcmeehan reviewed Apr 2, 2019

View reviewed changes

mbasmanova force-pushed the refactor-aria-rules branch from b5207bc to 667b3cb Compare April 2, 2019 15:29

tdcmeehan reviewed Apr 2, 2019

View reviewed changes

mbasmanova force-pushed the refactor-aria-rules branch 3 times, most recently from 9c50fed to 5963ccf Compare April 2, 2019 19:46

tdcmeehan reviewed Apr 2, 2019

View reviewed changes

tdcmeehan approved these changes Apr 2, 2019

View reviewed changes

mbasmanova force-pushed the refactor-aria-rules branch from 5963ccf to ea81487 Compare April 3, 2019 00:24

mbasmanova merged commit 51a2e30 into prestodb:aria-scan-research Apr 3, 2019

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

mbasmanova deleted the refactor-aria-rules branch May 24, 2019 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial refactoring of pushdown subscripts rule #12534

Initial refactoring of pushdown subscripts rule #12534

mbasmanova commented Mar 26, 2019

mbasmanova commented Mar 28, 2019

tdcmeehan Apr 2, 2019

mbasmanova Apr 2, 2019

tdcmeehan Apr 2, 2019

tdcmeehan Apr 2, 2019

mbasmanova Apr 2, 2019

tdcmeehan Apr 2, 2019 •

edited

Loading

mbasmanova Apr 2, 2019

tdcmeehan Apr 2, 2019

tdcmeehan Apr 2, 2019

tdcmeehan Apr 2, 2019

tdcmeehan Apr 2, 2019

mbasmanova Apr 2, 2019

tdcmeehan Apr 2, 2019

mbasmanova Apr 2, 2019

tdcmeehan Apr 2, 2019

mbasmanova Apr 2, 2019

tdcmeehan Apr 2, 2019

mbasmanova Apr 2, 2019

tdcmeehan Apr 2, 2019

mbasmanova commented Apr 2, 2019

tdcmeehan Apr 2, 2019

mbasmanova Apr 2, 2019

tdcmeehan Apr 2, 2019 •

edited

Loading

tdcmeehan Apr 2, 2019

mbasmanova Apr 2, 2019

mbasmanova commented Apr 2, 2019

tdcmeehan Apr 2, 2019

tdcmeehan Apr 2, 2019

mbasmanova Apr 2, 2019

tdcmeehan left a comment

mbasmanova commented Apr 3, 2019

Initial refactoring of pushdown subscripts rule #12534

Initial refactoring of pushdown subscripts rule #12534

Conversation

mbasmanova commented Mar 26, 2019

mbasmanova commented Mar 28, 2019

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 Apr 2, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdcmeehan Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova commented Apr 2, 2019

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

mbasmanova commented Apr 3, 2019

tdcmeehan Apr 2, 2019 •

edited

Loading

tdcmeehan Apr 2, 2019 •

edited

Loading