>From Glenn Galvizo <[email protected]>: Glenn Galvizo 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 1: (38 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/DefaultRuleSetFactory.java@79 PS1, 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/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java@356 PS1, 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/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/Cost.java@43 PS1, Line 43: public Cost MAXCost() { change to maxCost (just to be in style) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/ICost.java@25 PS1, Line 25: ICost MAXCost(); rename to maxCost (just to be in style) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@566 PS1, 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/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java@193 PS1, 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/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceJoinAccessMethodRule.java@159 PS1, 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/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java@199 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? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java@485 PS1, Line 485: //System.out.println("2 choosenIndexes " + chosenIndexes.size() + " " + joinEnumCode); remove (or use logger) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java@495 PS1, Line 495: //System.out.println("Path 2 " + joinEnumCode); remove (or use logger) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceSelectAccessMethodRule.java@509 PS1, Line 509: //System.out.println("Path 3 " + joinEnumCode); remove (or use logger) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinCondition.java@25 PS1, Line 25: ILogicalExpression joinCondition; seems like all these fields should be public https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@134 PS1, Line 134: cardMap = new HashMap<>(); what is temp code? 😊 can you make this into a TODO comment? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@562 PS1, Line 562: remove comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@713 PS1, Line 713: remove comment if there are no plans of using this method https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@821 PS1, Line 821: //EnumerateJoinsRule.printPlan(pp, (AbstractLogicalOperator) op, "Original Whole plan in JN 2"); log the plan here (if you are not logging it elsewhere) and remove comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@827 PS1, Line 827: //EnumerateJoinsRule.printPlan(pp, this.op, "Original Whole plan in JN END"); remove comments (or use a logger) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@833 PS1, Line 833: if (cheapestPlanIndex > 0) { remove comments (or use a logger) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@131 PS1, Line 131: //System.out.println("applicable Join Conditions size is 0 in join Node " + this.jnArrayIndex); remove comment or use a logger https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@247 PS1, Line 247: IntroduceJoinAccessMethodRule tmp = new IntroduceJoinAccessMethodRule(); > I'm not sure about creating/including a rule here at this stage? Ali and > Glenn probably have a bette […] there's an interface in the org.apache.asterix.optimizer.rules.am.array package called IIntroduceAccessMethodRuleLocalRewrite that seems like it could see some use here... maybe look at that? (and move the interface outside of *.am.array to *.am). https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@253 PS1, 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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@285 PS1, 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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@751 PS1, Line 751: //cpPlan = buildCPJoinPlan(leftJn, rightJn, nestedLoopJoinExpr); remove comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@781 PS1, Line 781: void dump() { change to toString (for better integration with IDEs and for logging) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@843 PS1, Line 843: void printCostOfAllPlans() { change to either return a value or use a logger (avoid System.out) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/PlanNode.java@32 PS1, Line 32: int allPlansIndex; mark fields below as private and make getter methods as needed https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/PlanNode.java@180 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 https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@232 PS1, Line 232: import java.util.regex.Matcher; 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/+/17143/1/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@661 PS1, Line 661: //Pattern number = Pattern.compile("\\d+\\.\\d+|\\d+"); remove comment https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@677 PS1, Line 677: /* remove comment if not needed https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@690 PS1, Line 690: //Matcher matCHP = singleHintPattern.matcher(hintParams); remove comment if not needed https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/base/OperatorAnnotations.java@32 PS1, 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/+/17143/1/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/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/ConstantExpression.java@134 PS1, 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/+/17143/1/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/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/HashJoinExpressionAnnotation.java@24 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 https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/JoinProductivityAnnotation.java@23 PS1, Line 23: double productivity = 1.0; mark both as private https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/PredicateCardinalityAnnotation.java@23 PS1, 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/+/17143/1/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/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/CardHints.java@29 PS1, Line 29: List<List<String>> listRefNames; mark all as private https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/HeuristicOptimizer.java@70 PS1, 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/+/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: 1 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-Comment-Date: Fri, 19 Aug 2022 16:59:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Wail Alkowaileet <[email protected]> Gerrit-MessageType: comment
