Change in asterixdb[master]: [NO-ISSUE][COMP] Avoid adding redundant var in AbstractIntro...

["Dmitry Lychagin (Code Review)" <do...@asterixdb.incubator.apache.org>]

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 <xk...@gmail.com>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Dmitry Lychagin <dm...@couchbase.com>
Gerrit-Reviewer: Jenkins <je...@fulliautomatix.ics.uci.edu>
Gerrit-HasComments: Yes