[WIP] Add rewriter for multiple similar APPROX_PERCENTILEs in same clause #14

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 rongrong and prithvip 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add rewriter for multiple similar APPROX_PERCENTILEs in same clause #14

[WIP] Add rewriter for multiple similar APPROX_PERCENTILEs in same clause #14

jnmugerwa commented Mar 11, 2021 •

edited

Loading

kaikalur left a comment

kaikalur Mar 11, 2021

jnmugerwa Mar 12, 2021

kaikalur Mar 11, 2021

jnmugerwa Mar 12, 2021 •

edited

Loading

kaikalur Mar 11, 2021

jnmugerwa Mar 12, 2021

kaikalur Mar 11, 2021

jnmugerwa Mar 12, 2021

kaikalur Mar 11, 2021

jnmugerwa Mar 12, 2021

kaikalur Mar 29, 2021

jnmugerwa Mar 29, 2021

kaikalur Mar 29, 2021 •

edited

Loading

kaikalur Mar 30, 2021

jnmugerwa Apr 1, 2021

kaikalur Mar 29, 2021

jnmugerwa Mar 29, 2021

kaikalur Mar 30, 2021

jnmugerwa Apr 1, 2021

kaikalur Mar 29, 2021

jnmugerwa Mar 29, 2021

kaikalur Mar 29, 2021

jnmugerwa Apr 1, 2021

kaikalur Mar 29, 2021

jnmugerwa Mar 29, 2021

kaikalur Mar 29, 2021

jnmugerwa Apr 1, 2021

kaikalur Mar 29, 2021

jnmugerwa Apr 1, 2021

kaikalur Mar 30, 2021

kaikalur Mar 30, 2021

[WIP] Add rewriter for multiple similar APPROX_PERCENTILEs in same clause #14

Are you sure you want to change the base?

[WIP] Add rewriter for multiple similar APPROX_PERCENTILEs in same clause #14

Conversation

jnmugerwa commented Mar 11, 2021 • edited Loading

kaikalur 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

jnmugerwa Mar 12, 2021 • 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

kaikalur Mar 29, 2021 • 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

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

jnmugerwa commented Mar 11, 2021 •

edited

Loading

jnmugerwa Mar 12, 2021 •

edited

Loading

kaikalur Mar 29, 2021 •

edited

Loading