>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 7: (35 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/DefaultRuleSetFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/b3052d87_ca5c4afc 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 […] Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/355741e1_82939f4a PS1, Line 356: dataExchange.add(new ConsolidateSelectsRule()); > move these rules to a new collection, this collection should only be for data > exchange rules Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/Cost.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/e3de075f_0a66c88d PS1, Line 24: cost > It seems "cost" could be final Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/2d8852bb_0c58e7d5 PS1, Line 43: public Cost MAXCost() { > change to maxCost (just to be in style) Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/ICost.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/362a8c8d_408007e6 PS1, Line 25: ICost MAXCost(); > rename to maxCost (just to be in style) Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/512de97a_db25b959 PS1, Line 86: /* : MetadataProvider metadataProvider = (MetadataProvider) context.getMetadataProvider(); : ICcApplicationContext ccAppContext = metadataProvider.getApplicationContext(); : CompilerProperties compilerProperties = ccAppContext.getCompilerProperties(); : return compilerProperties.isCBOEnabled(); : */ > remove Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/2133ebb1_49dc7798 PS1, Line 127: OnlyOneAssign > "onlyOneAssign" Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/3417496d_aa5e7cd4 PS1, Line 128: equals > == could be used instead of equals() Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/c884baa4_f7a0a5fc PS1, Line 134: equals > == could be used instead of equals() Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/364ecff2_87c78841 PS1, Line 304: equals > == could be used instead of equals() Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/02d59367_07b1bdf7 PS1, Line 312: printPlan > Should use Logger instead of System.out […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/b75b1d22_6a1eeffd PS1, Line 566: //printPlan(pp, (AbstractLogicalOperator) joinOps.get(0), "New Whole Plan after buildNewTree"); > remove comments or rewrite these to use a logger 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/fb13ec80_24c13c48 PS1, Line 485: //System.out.println("2 choosenIndexes " + chosenIndexes.size() + " " + joinEnumCode); > remove (or use logger) Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/40935d62_8aed5fd8 PS1, Line 495: //System.out.println("Path 2 " + joinEnumCode); > remove (or use logger) Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/3c9593dc_c75dcbbc PS1, Line 509: //System.out.println("Path 3 " + joinEnumCode); > remove (or use logger) 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/c4d2e009_c0f30041 PS1, Line 76: JoinEnum > Use Map<> instead of HashMap<>. This occurred in several place (e.g. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/272f059c_7f120f31 PS1, Line 134: cardMap = new HashMap<>(); > what is temp code? 😊 can you make this into a TODO comment? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/87de9f1a_5a6ef70e PS1, Line 203: HashSet<LogicalVariable> vars = new HashSet<>(); > Declare outside the loop and use clear instead of creating a new one each > time. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/11a0f758_3e8ecdee PS1, Line 203: HashSet > Set<> instead of HashSet<> Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/fd074cef_3896637c PS1, Line 562: > remove comment Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/0c8d7904_3d3ecd71 PS1, Line 713: > remove comment if there are no plans of using this method Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/b8368004_22e2dfbb 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 Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/e3f1f30c_d1d26926 PS1, Line 827: //EnumerateJoinsRule.printPlan(pp, this.op, "Original Whole plan in JN END"); > remove comments (or use a logger) Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/83bb6741_4f921f70 PS1, Line 833: if (cheapestPlanIndex > 0) { > remove comments (or use a logger) 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/533b451e_cd933b8c PS6, Line 637: int lastBaseLevelJnNum = initializeBaseLevelJoinNodes(); > i think there is a problem here. initializeBaseLevelJoinNodes() can return > JoinNode. […] Removed JoinNode.NO_CARDS as the return value. initializeBaseLevelJoinNodes() now only returns PlanNode.NO_PLAN if it cannot create a plan, so there is no problem anymore. This is better than returning true/false. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/e7573b99_9c890b81 PS6, Line 701: if (scanOp == null) { : continue; // what happens to the cards and sizes then? this may happen in case of in lists : } > i don't think this will ever happen. […] 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/25af839a_3b6266f7 PS1, Line 131: //System.out.println("applicable Join Conditions size is 0 in join Node " + this.jnArrayIndex); > remove comment or use a logger Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/dcea0194_0abc12f3 PS1, Line 253: private boolean subset(int one, int two) { // one is a subset of two > refactor to return (one & two) == one Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/d20ca46d_0ef71cd3 PS1, Line 285: //System.out.println("newTable Bits == 0"); > remove comment (and if statement) if not needed or use a logger Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/2ef9339a_c394c235 PS1, Line 751: //cpPlan = buildCPJoinPlan(leftJn, rightJn, nestedLoopJoinExpr); > remove comment Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/df09f317_05083298 PS1, Line 843: void printCostOfAllPlans() { > change to either return a value or use a logger (avoid System. […] 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/43bea557_7fcb5ca3 PS6, Line 319: joinEnum.getCostMethodsHandle().costFullScan(this); > we could replace this with "opCost" since we already calculated the cost (to > avoid calculating it ag […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/9daa5dac_54aa327d PS6, Line 348: costFullScan(this); > isn't this supposed to be costIndexScan(this);? 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/6470a997_3d61bbf3 PS1, Line 31: static > Remove static. Done 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/comment/750532bd_ae5d1c46 PS1, Line 70: EstimatedCostComputationVisitor estCostCompVisitor = new EstimatedCostComputationVisitor(); > this should be in an optimization set , wrap this in a rule 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: 7 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: Thu, 08 Sep 2022 17:13:14 +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
