[WIP] Add rewriter for multiple similar APPROX_PERCENTILEs in same clause by jnmugerwa · Pull Request #14 · prestodb/sql

jnmugerwa · 2021-03-11T03:27:25Z

Added a rewriter for the following anti-pattern:

-> Query has multiple APPROX_PERCENTILEs in the same clause
-> The first argument for each APPROX_PERCENTILE is identical
-> The second argument for each APPROX_PERCENTILE is a literal

kaikalur

So you should move lot of this logic to actual visit methods.

kaikalur · 2021-03-11T04:05:36Z

...main/java/com/facebook/coresql/rewriter/approxpercentile/ApproxPercentilePatternMatcher.java

+
+    public ApproxPercentilePatternMatcher()
+    {
+        selectMap = new HashMap<>();


Look at the Presto coding style. We use ImmutableMap.Builder.

We now return an immutable map.

Done

kaikalur · 2021-03-11T04:07:29Z

...main/java/com/facebook/coresql/rewriter/approxpercentile/ApproxPercentilePatternMatcher.java

+    private boolean isLiteral(AstNode node)
+    {
+        return node.getId() == JJTUNSIGNEDNUMERICLITERAL;
+    }


for now, just move this method into ASTNode. Or better yet have a util class - NodeUtil that has all these static functions. Also, name the function to reflect what it does - like isUnsignedNumericLiteral

Left in this class for now -- will factor this and similar utility methods into a NodeUtil class in a NodeUtil-specific PR.

Done

kaikalur · 2021-03-11T04:08:42Z

...main/java/com/facebook/coresql/rewriter/approxpercentile/ApproxPercentilePatternMatcher.java

+
+    private void processSelectItem(AstNode node, Map<String, List<AstNode>> map)
+    {
+        AstNode functionCall = node.GetFirstChildOfKind(JJTFUNCTIONCALL);


So it will be good to generalize it to handle approx percentiles anywhere in the expression. Best to do this in visitFunctionCall

kaikalur · 2021-03-11T04:09:29Z

...main/java/com/facebook/coresql/rewriter/approxpercentile/ApproxPercentilePatternMatcher.java

+            return;
+        }
+        AstNode identifier = functionCall.GetFirstChildOfKind(JJTIDENTIFIER);
+        if (identifier == null || !identifier.GetImage().equalsIgnoreCase("APPROX_PERCENTILE")) {


Add a utilitiy method - isApproxPercentile and do this check there.

kaikalur · 2021-03-11T04:10:53Z

...main/java/com/facebook/coresql/rewriter/approxpercentile/ApproxPercentilePatternMatcher.java

+            return;
+        }
+        map.putIfAbsent(firstArg.GetImage(), new LinkedList<>());
+        map.get(firstArg.GetImage()).add(node);


Instead of getImage, use Unparser.unparse(firstArg) so it will be general.

kaikalur · 2021-03-29T22:58:14Z

rewriter/src/main/java/com/facebook/coresql/rewriter/ApproxPercentileRewriter.java

+public class ApproxPercentileRewriter
+        extends Rewriter
+{
+    private static final int ZERO_INDEXING_OFFSET = 1;


Confusing naming.

APPROX_PERCENTILE is 1-indexed while the data structure we store our percentiles in is 0-indexing, making this offset necessary. I feel like this name is descriptive given that context?

kaikalur · 2021-03-29T22:58:30Z

rewriter/src/main/java/com/facebook/coresql/rewriter/ApproxPercentileRewriter.java

+        extends Rewriter
+{
+    private static final int ZERO_INDEXING_OFFSET = 1;
+    private static final String REPLACEMENT_FORMAT = " APPROX_PERCENTILE(%s, ARRAY%s)[%d]";


Maybe use ARRAY[%s]

Also don't add space padding.

kaikalur · 2021-03-29T23:00:12Z

rewriter/src/main/java/com/facebook/coresql/rewriter/ApproxPercentileRewriter.java

+                .anyMatch(key -> firstArgToApproxPercentileNode.get(key).size() >= 2);
+    }
+
+    private boolean canApplyApproxPercentileRewrite(AstNode node)


It should be more like: needsApproxPercentileRewrite

Maybe needsRewrite and make it take more specific type: FunctionCall

kaikalur · 2021-03-29T23:00:59Z

rewriter/src/main/java/com/facebook/coresql/rewriter/ApproxPercentileRewriter.java

+        return firstArgToPercentiles.get(firstArg).indexOf(secondArg) + ZERO_INDEXING_OFFSET;
+    }
+
+    private static Optional<AstNode> getNthArgument(AstNode node, int n)


Can you move this to AstNode?

Will be moved to NodeUtils in a future PR.

kaikalur · 2021-03-29T23:01:40Z

rewriter/src/main/java/com/facebook/coresql/rewriter/ApproxPercentileRewriter.java

+            return false;
+        }
+        Optional<String> image = Optional.ofNullable(identifier.get().GetImage());
+        return image.isPresent() && image.get().equalsIgnoreCase("APPROX_PERCENTILE");


Use a static final constant for the name of the function.

kaikalur · 2021-03-29T23:02:21Z

rewriter/src/main/java/com/facebook/coresql/rewriter/ApproxPercentileRewriter.java

+        return getNthArgument(node, 1).filter(astNode -> astNode.getId() == JJTUNSIGNEDNUMERICLITERAL).isPresent();
+    }
+
+    private static boolean isApproxPercentileNode(AstNode node)


Is it needed here? You should move it to the pattern matcher class.

Will be moved to NodeUtils in a future PR.

kaikalur · 2021-03-29T23:02:53Z

rewriter/src/main/java/com/facebook/coresql/rewriter/ApproxPercentileRewriter.java

+        {
+            if (isApproxPercentileNode(node) && hasUnsignedLiteralSecondArg(node)) {
+                String firstArg = unparse(getNthArgument(node, 0).get()).trim();
+                String secondArg = unparse(getNthArgument(node, 1).get()).trim();


You don't need to unparser the second argument?

Agreed -- removed unparsing when we deal with secondArg.

kaikalur · 2021-03-29T23:05:51Z

rewriter/src/test/java/com/facebook/coresql/rewriter/TestApproxPercentileRewriter.java

+            "SELECT APPROX_PERCENTILE(x, ARRAY[0.1, 0.2, 0.3]) FROM (SELECT APPROX_PERCENTILE(x, 0.1) from T);"
+    };
+
+    private static final ImmutableMap<String, String> STATEMENT_TO_REWRITTEN_STATEMENT =


You can simply have two arrays - one original sql and other one positionally matching rewritten sql. Or just array of pairs.

Implemented two arrays.

Done.

kaikalur · 2021-03-30T16:11:38Z

rewriter/src/main/java/com/facebook/coresql/rewriter/ApproxPercentileRewriter.java

+        extends Rewriter
+{
+    private static final int ZERO_INDEXING_OFFSET = 1;
+    private static final String REPLACEMENT_FORMAT = " APPROX_PERCENTILE(%s, ARRAY%s)[%d]";


Also don't add space padding.

kaikalur · 2021-03-30T16:13:49Z

rewriter/src/main/java/com/facebook/coresql/rewriter/ApproxPercentileRewriter.java

+                .anyMatch(key -> firstArgToApproxPercentileNode.get(key).size() >= 2);
+    }
+
+    private boolean canApplyApproxPercentileRewrite(AstNode node)


Maybe needsRewrite and make it take more specific type: FunctionCall

jnmugerwa requested a review from kaikalur March 11, 2021 03:32

kaikalur suggested changes Mar 11, 2021

View reviewed changes

jnmugerwa force-pushed the rewrite/approx-percentile branch from 30dc75e to a8c3d49 Compare March 12, 2021 18:03

jnmugerwa requested a review from kaikalur March 12, 2021 18:09

jnmugerwa force-pushed the rewrite/approx-percentile branch from a8c3d49 to 4a46b4c Compare March 15, 2021 18:05

jnmugerwa requested review from prithvip and rongrong March 16, 2021 17:56

jnmugerwa force-pushed the rewrite/approx-percentile branch from 4a46b4c to a1aaeea Compare March 16, 2021 18:06

jnmugerwa changed the title ~~[WIP] Add rewriter for multiple similar APPROX_PERCENTILEs in SELECT clause~~ [WIP] Add rewriter for multiple similar APPROX_PERCENTILEs in same clause Mar 16, 2021

jnmugerwa force-pushed the rewrite/approx-percentile branch 2 times, most recently from f7444b2 to 92a67b6 Compare March 16, 2021 20:09

jnmugerwa force-pushed the rewrite/approx-percentile branch 2 times, most recently from eea9344 to b4db47a Compare March 29, 2021 22:52

kaikalur suggested changes Mar 29, 2021

View reviewed changes

kaikalur suggested changes Mar 30, 2021

View reviewed changes

jnmugerwa force-pushed the rewrite/approx-percentile branch 3 times, most recently from e251722 to e8204c4 Compare April 1, 2021 04:01

jnmugerwa requested a review from kaikalur April 1, 2021 15:33

Add rewriter for multiple similar APPROX_PERCENTILEs in same clause

85ae620

kaikalur force-pushed the rewrite/approx-percentile branch from e8204c4 to 85ae620 Compare April 13, 2021 17:58

Conversation

jnmugerwa commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaikalur left a comment

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!

jnmugerwa Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaikalur Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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

Projects

Milestone

Development

Uh oh!

jnmugerwa commented Mar 11, 2021 •

edited

Loading

jnmugerwa Mar 12, 2021 •

edited

Loading

kaikalur Mar 29, 2021 •

edited

Loading