Dmitry Lychagin has posted comments on this change. Change subject: [NO-ISSUE][COMP] Avoid adding redundant var in AbstractIntroduceGroupByCombinerRule ......................................................................
Patch Set 4: (4 comments) https://asterix-gerrit.ics.uci.edu/#/c/2933/4/asterixdb/asterix-app/src/test/resources/log4j2-asterixdb-test.xml File asterixdb/asterix-app/src/test/resources/log4j2-asterixdb-test.xml: Line 44: <Logger name="org.apache.hyracks.algebricks" level="TRACE"> Let's revert these lines. We don't need query plans in the logs by default. https://asterix-gerrit.ics.uci.edu/#/c/2933/4/asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/group-by/redundant-var-in-groupby/redundant-var-in-groupby.1.ddl.sqlpp File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/group-by/redundant-var-in-groupby/redundant-var-in-groupby.1.ddl.sqlpp: Line 38: LET nearby_buildings = (select r.buildingType as Religion, count(r) as cnt why "as Religion"? can we remove it? This is building type, so let this field be called buildingType https://asterix-gerrit.ics.uci.edu/#/c/2933/4/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/AbstractIntroduceGroupByCombinerRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/AbstractIntroduceGroupByCombinerRule.java: Line 87: if (!newGbyLiveVars.contains(usedDecorVars.get(0))) { can we extract usedDecorVars.get(0) into a separate variable and use this variable in this line and the new line (i.e. newGbyLiveVars.add(usedDecorVar) ) Line 108: if (!propagatedVars.contains(var) && !newGbyLiveVars.contains(var)) { The only difference between VariableUtilities.getLiveVariables(newGbyOp) and VariableUtilities.getProducedVariables(newGbyOp) seems to be those decor vars that are just propagated through the group by without mapping. They are reported as live variables, but are not reported as produced variables because gby does not produce them. We've already got live variables in line 81 and are adding propagated decor vars to it in your new line (91), so I think you can remove !propagatedVars.contains(var) in this line and only check whether the free var is not contained in newGbyLiveVars (i.e. just do !newGbyLiveVars.contains(var) in this if). This also means that propagatedVars don't need to be computed at all and lines 101-102 can be removed. -- To view, visit https://asterix-gerrit.ics.uci.edu/2933 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic1ab9aee31db95d5782385bc3d53777da54f6d83 Gerrit-PatchSet: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Xikui Wang <xkk...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <dmitry.lycha...@couchbase.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-HasComments: Yes
