>From Vijay Sarathy <[email protected]>: Attention is currently required from: Wail Alkowaileet, Ali Alsuliman, Glenn Galvizo. Vijay Sarathy has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143 )
Change subject: [ASTERIXDB-3046][COMP] Support cost based query optimization. ...................................................................... Patch Set 10: (17 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/c115f854_29d0372a PS4, Line 551: Index index = joinEnum.findSampleIndex(scanOp, context); > "findSampleIndex" does not seem to be related to joinEnum. […] Moved this out to Stats.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/69e03b8c_9c803fe0 PS6, Line 152: List<PlanNode> allPlans = joinEnum.getAllPlans(); > it seems that joinEnum knows internally where the cheapest plan is. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/ee1867ee_55bccecb PS6, Line 154: joinEnum.enumerateJoins(); > it's usually not safe to have methods return an integer that is used like a > boolean. […] This is ok, as enumerateJoins() returns a valid plan (number > 0) or NO_PLAN (-1), and the return value is compared with NO_PLAN in only one place in the code. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/675f2d40_9e6b147f PS10, Line 369: joinEnumCode > rename similar to the other one; checkApplicableOnly Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/PlanNode.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/42f6e51e_fccfee6c PS10, Line 106: if (getLeftPlanIndex() == NO_PLAN && getRightPlanIndex() == NO_PLAN) { : return true; : } : return false; > No need for the "if" here: […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/8abd83d5_10890fbd PS10, Line 113: if (getLeftPlanIndex() != NO_PLAN && getRightPlanIndex() != NO_PLAN) { : return true; : } : return false; > Same as above Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinCondition.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/bad10797_5a680a6d PS1, Line 20: util > Let's have EnumerateJoinsRule, JoinCondition, JoinNode, JoinEnum, PlanNode, > and StatsUtil under a ne […] Done File asterixdb/asterix-app/src/main/resources/cc2.conf: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/0b2fff29_2e27696d PS7, Line 52: compiler.queryplanshape=zigzag > i don't think this is picked up. see my comment in EnumerateJoinsRule. Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/AbstractFunctionCallExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/46f57369_d113e375 PS10, Line 267: setAnnotation > AbstractFunctionCallExpression is kind of generic, and the implementation > here seems to be very spec […] Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/acf19cda_c6a3aa69 PS10, Line 129: /* : for (Map.Entry<String, Object> anno : op.getAnnotations().entrySet()) { > remove commented code (you can just roll back the whole file) Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitorJson.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/736b6d98_708b8f94 PS10, Line 215: equals > you can use == for tags, same thing for the one below it. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/b2f39fb7_afcd42cd PS10, Line 237: > revert adding extra line Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/util/OperatorManipulationUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/e58e5b43_4c1532e8 PS10, Line 527: //ScalarFunctionCallExpression scalarExpr = (ScalarFunctionCallExpression) joinExpr; > remove commented line. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/8017c490_27cd57b1 PS10, Line 529: if (arg.getValue().getExpressionTag().equals(LogicalExpressionTag.FUNCTION_CALL)) { > we can use == for tags comparison: […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/3a5cd3f6_b9c761dc PS10, Line 534: // cloning is needed according to Dmitry. > remove comment. Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/PhysicalOptimizationConfig.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/f29fbd4f_73602730 PS10, Line 55: public static double selectivityForSecondaryIndexSelection = 0.1; > we should move this to a better place. move it to Stats.java for now. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/3beba203_068c3913 PS10, Line 251: // default value if option is not in ("on", "off", "test") > we can remove these comments since the code is clear about it Done -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I848adf6a8fdcfea360655ab649de2fb75a73c814 Gerrit-Change-Number: 17143 Gerrit-PatchSet: 10 Gerrit-Owner: Vijay Sarathy <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Glenn Galvizo <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Wail Alkowaileet <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Attention: Wail Alkowaileet <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Attention: Glenn Galvizo <[email protected]> Gerrit-Comment-Date: Tue, 13 Sep 2022 19:50:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Wail Alkowaileet <[email protected]> Comment-In-Reply-To: Ali Alsuliman <[email protected]> Gerrit-MessageType: comment
