>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 8: (20 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/SqlppCompilationProvider.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/c40c6d48_c41c5aee PS1, Line 93: cardinality > Probably should be declared as a constant -- similar to > "COMPILER_FORCEJOINORDER_KEY" Might be removed, so will defer this to later. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/2d4ffc93_974e6035 PS6, Line 445: true > shouldn't we check that "expr" is an EQ? changed the logic to actually look for conjuncts of EQs File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/3f3a3bb2_4ccf556b PS1, Line 193: * Simply picks the first index that it finds. TODO: Improve this decision > remove this comment? (since we are now more "systematic" 😊 ) This method still picks the first one, so will leave the comment for now. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceJoinAccessMethodRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/9807b3c8_bee6f7c8 PS1, Line 159: public boolean rewritePre2(Mutable<ILogicalOperator> opRef, IOptimizationContext context) > is there a better name for this method? Done 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/75851cf0_33d94d68 PS1, Line 199: public boolean rewritePre2(Mutable<ILogicalOperator> opRef, IOptimizationContext context, > same as the JOIN rule, can you use a more descriptive method name here? 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/c0d73a0e_15abf214 PS1, Line 25: ILogicalExpression joinCondition; > seems like all these fields should be public Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/4f3098e6_c9600571 PS6, Line 807: CompilerProperties.COMPILER_FORCE_JOIN_ORDER_KEY > are we planning to make this property configurable by user at the application > level so that the user […] Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/85eeca49_ca600bf3 PS1, Line 781: void dump() { > change to toString (for better integration with IDEs and for logging) Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/PlanNode.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/68d85762_60b52f4e PS1, Line 32: int allPlansIndex; > mark fields below as private and make getter methods as needed Deferred for later as changes are extensive. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/f50927c3_70ea2db2 PS1, Line 180: public void displayPlan(int indent) { > make to toString method, and if we need to print our plan we should use a > logger removed it. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/StatsUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/4e1b3f42_52f671e5 PS1, Line 39: StatsUtil > Rename it to Stats or CBOStats instead of StatsUtil. Usually "util" classes > cannot be instantiated. Done File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/318f4dbc_e6e86a0c PS1, Line 661: //Pattern number = Pattern.compile("\\d+\\.\\d+|\\d+"); > remove comment Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/6b830a23_3f6b490e PS1, Line 677: /* > remove comment if not needed Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/01b3d447_b1f48fab PS1, Line 690: //Matcher matCHP = singleHintPattern.matcher(hintParams); > remove comment if not needed Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/base/OperatorAnnotations.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/4c7a140c_b6c97351 PS1, Line 32: public static final String OP_INPUT_CARDINALITY = "input_cardinality"; > use upper case (to match style of other annotations) Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/HashJoinExpressionAnnotation.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/e606f036_5e8321f4 PS1, Line 24: String buildAlias = null; // an alias to indicate which side is to be used to build; could be the left side or the right side. > mark as private and make a getter method Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/JoinProductivityAnnotation.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/1e74e99d_cee4b2ff PS1, Line 23: double productivity = 1.0; > mark both as private Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/PredicateCardinalityAnnotation.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/93f3a3f5_49098787 PS1, Line 23: Double selectivity; > mark as private — also we shouldn’t need to box this (use the primitive > double) instead Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/CardHints.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/b8fc5d19_863f2b4e PS1, Line 29: List<List<String>> listRefNames; > mark all as private 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/bf4ff1d3_eff79852 PS1, Line 304: private void setString(String property, String value) { : properties.setProperty(property, value); : } : : private String getString(String property, String defaultValue) { : String value = properties.getProperty(property); : return value == null ? defaultValue : value; : } > This shouldn't be needed if we use true/false This is now needed for query plan shape. -- 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: 8 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: Fri, 09 Sep 2022 22:50:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Wail Alkowaileet <[email protected]> Comment-In-Reply-To: Ali Alsuliman <[email protected]> Comment-In-Reply-To: Glenn Galvizo <[email protected]> Gerrit-MessageType: comment
