>From Glenn Galvizo <ggalv...@uci.edu>: Glenn Galvizo has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604 )
Change subject: [ASTERIXDB-3046][COMP] Changes to support cost based query optimization in Asterix DB. ...................................................................... Patch Set 45: (33 comments) initial set of comments, i’ll make another pass later in the week https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/DefaultRuleSetFactory.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/DefaultRuleSetFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/DefaultRuleSetFactory.java@79 PS45, Line 79: defaultLogicalRewrites.add(new Pair<>(seqOnceCtrl, RuleCollections.buildFulltextContainsRuleCollection())); what is the justification for moving the full-text rules here? (see the comment about making the CBO stuff it’s own rule collection) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java@356 PS45, Line 356: dataExchange.add(new ConsolidateSelectsRule()); move these rules to a new collection, this collection should only be for data exchange rules https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/Cost.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/Cost.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/Cost.java@43 PS45, Line 43: public Cost MAXCost() { change to maxCost (just to be in style) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/ICost.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/ICost.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/ICost.java@25 PS45, Line 25: ICost MAXCost(); rename to maxCost https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@566 PS45, Line 566: //printPlan(pp, (AbstractLogicalOperator) joinOps.get(0), "New Whole Plan after buildNewTree"); remove comments or rewrite these to use a logger https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java@193 PS45, Line 193: * Simply picks the first index that it finds. TODO: Improve this decision remove this comment? (since we are now more systematic :-)) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceJoinAccessMethodRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceJoinAccessMethodRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceJoinAccessMethodRule.java@159 PS45, Line 159: public boolean rewritePre2(Mutable<ILogicalOperator> opRef, IOptimizationContext context) is there a better name for this method? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java@199 PS45, Line 199: public boolean rewritePre2(Mutable<ILogicalOperator> opRef, IOptimizationContext context, same as the JOIN rule, can you use a more descriptive method name here? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java@485 PS45, Line 485: //System.out.println("2 choosenIndexes " + chosenIndexes.size() + " " + joinEnumCode); remove https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java@489 PS45, Line 489: //System.out.println("Path 1 " + joinEnumCode); remove https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java@495 PS45, Line 495: //System.out.println("Path 2 " + joinEnumCode); remove https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java@509 PS45, Line 509: //System.out.println("Path 3 " + joinEnumCode); remove https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinCondition.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinCondition.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinCondition.java@25 PS45, Line 25: ILogicalExpression joinCondition; seems like all these fields should be public https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@131 PS45, Line 131: // TEMP CODE what is temp code? :-) can you make this into a TODO comment? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@561 PS45, Line 561: /* remove comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@713 PS45, Line 713: public ILogicalOperator findDataSourceScanOperatorParent(ILogicalOperator op) { remove comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@830 PS45, Line 830: /* log the plan here (if you are not logging it elsewhere) and remove comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@253 PS45, Line 253: private boolean subset(int one, int two) { // one is a subset of two refactor to return (one & two) == one https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@285 PS45, Line 285: //System.out.println("newTable Bits == 0"); remove comment (and if statement) if not needed or use a logger https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@738 PS45, Line 738: //else remove comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/PlanNode.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/PlanNode.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/PlanNode.java@31 PS45, Line 31: private static JoinEnum joinEnum; mark fields below as private and make getter methods as needed https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/PlanNode.java@180 PS45, Line 180: public void displayPlan(int indent) { this might be better as a toString method, and if we need to print our plan we should use a logger https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@654 PS45, Line 654: private IExpressionAnnotation parseExpressionAnnotation(Token hintToken) { shouldn’t be using regex package in the grammar file (feels backwards, also there are methods we have for this stuff ) — see spatial join hint below to see how they parse numbers https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@659 PS45, Line 659: //Pattern number = Pattern.compile("\\d+\\.\\d+|\\d+"); remove comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@676 PS45, Line 676: Matcher matCHP = singleHintPattern.matcher(hintParams); remove comment if not needed https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@688 PS45, Line 688: //Matcher matCHP = singleHintPattern.matcher(hintParams); remove comment if not needed https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/base/OperatorAnnotations.java 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/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/base/OperatorAnnotations.java@32 PS45, Line 32: public static final String OP_INPUT_CARDINALITY = "input_cardinality"; use upper case (to match style of other annotations) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/ConstantExpression.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/ConstantExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/ConstantExpression.java@134 PS45, Line 134: return "NULL"; any reason for this change? (the other constant toString methods are still lowercase) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/HashJoinExpressionAnnotation.java 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/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/HashJoinExpressionAnnotation.java@24 PS45, 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 https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/JoinProductivityAnnotation.java 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/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/JoinProductivityAnnotation.java@23 PS45, Line 23: double productivity = 1.0; mark as private https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/PredicateCardinalityAnnotation.java 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/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/PredicateCardinalityAnnotation.java@23 PS45, Line 23: Double selectivity; mark as private — also we shouldn’t need to box this (use the primitive double) instead https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/CardHints.java 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/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/CardHints.java@29 PS45, Line 29: List<List<String>> listRefNames; mark as private https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/HeuristicOptimizer.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/HeuristicOptimizer.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/HeuristicOptimizer.java@70 PS45, Line 70: EstimatedCostComputationVisitor estCostCompVisitor = new EstimatedCostComputationVisitor(); this should be in an optimization set , wrap this in a rule -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604 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: I8d5fe4947df486d054fdf0f178fccf0c95eacfb6 Gerrit-Change-Number: 15604 Gerrit-PatchSet: 45 Gerrit-Owner: Vijay Sarathy <vijay.sara...@couchbase.com> Gerrit-Reviewer: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Glenn Galvizo <ggalv...@uci.edu> Gerrit-Reviewer: Ian Maxon <ima...@uci.edu> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Wail Alkowaileet <wael....@gmail.com> Gerrit-Reviewer: murali4...@gmail.com Gerrit-CC: Till Westmann <t...@couchbase.com> Gerrit-Comment-Date: Sat, 06 Aug 2022 23:52:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
