>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 12: (12 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/8d8ab65f_0e5d1d52 PS6, Line 70: private int totalNumberOfJoins; : private DataSourceScanOperator dataSourceOp; : private EmptyTupleSourceOperator emptyTupleSourceOp; > the way these variables are used suggests that they should be local > variables. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/39700012_02f4e710 PS6, Line 198: containsLeafInputOnly > what would happen if we encounter an operator that has more than one input > like UNION_ALL? this may […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/a2cce926_30328db0 PS6, Line 239: getJoinOpsAndLeafInputs > it's safer to make this method return true/false instead of passing a mutable > boolean and then havin […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/71acff21_1aa8c666 PS6, Line 356: SqlppHint.HASH_JOIN_HINT.getIdentifier() > why are we passing the hint identifier (which is "hashjoin")? Done File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/CompilerProperties.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/4de2095a_c153cb76 PS10, Line 276: public String getCBOMode() { : String cbo = accessor.getString(Option.COMPILER_CBO); : if (!(cbo.equals("on") || cbo.equals("off") || cbo.equals("test"))) : return AlgebricksConfig.CBO_DEFAULT; // default value if option is not in ("on", "off", "test") : return cbo; : } > we should follow the existing pattern that just returns the value from the > accessor: […] 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/57e5a416_a9a69f6d PS10, Line 32: public static final String OP_INPUT_CARDINALITY = "INPUT_CARDINALITY"; : public static final String OP_OUTPUT_CARDINALITY = "OUTPUT_CARDINALITY"; : public static final String OP_COST_TOTAL = "TOTAL_COST"; : public static final String OP_COST_LOCAL = "OP_COST"; : public static final String OP_LEFT_EXCHANGE_COST = "LEFT_EXCHANGE_COST"; : public static final String OP_RIGHT_EXCHANGE_COST = "RIGHT_EXCHANGE_COST"; > after moving EstimatedCostComputationVisitor to asterixdb layer, we can move > these to asterixdb laye […] 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/a892a5af_01627069 PS6, Line 24: String buildAlias = null; > i don't see this being used. Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/visitors/EstimatedCostComputationVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/091f6346_8aa5be1c PS10, Line 69: EstimatedCostComputationVisitor > we should move this to asterixdb layer (cbo package) so that it's with > AnnotateOperatorCostCardinali […] Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/config/AlgebricksConfig.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/2b8b31be_08bc51ab PS1, Line 38: public static final String CBO_DEFAULT = "on"; : public static final String FORCE_JOIN_ORDER_DEFAULT = "off" > could that be true/false instead of "on"/"off". […] 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/c8670973_ab5a47b8 PS1, Line 248: if (!(cbo.equals("on") || cbo.equals("off") || cbo.equals("test"))) > Make cbo to be a boolean instead to be consistent with other flags. […] 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/50ec7c58_1d4b9f59 PS10, Line 258: forceJoinOrderMode.equals("off") || forceJoinOrderMode.equals("old") : || forceJoinOrderMode.equals("new") > i think we need better names for users to understand the differences/behavior Removed options "new" and "old" to make it simpler. Now it is a boolean. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/4226466a_472146b1 PS10, Line 267: queryPlanShapeMode.equals("zigzag") || queryPlanShapeMode.equals("leftdeep") : || queryPlanShapeMode.equals("rightdeep") > we should have constants for these values. same thing for the other modes. […] 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: 12 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: Wed, 14 Sep 2022 17:54:16 +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
