>From Ali Alsuliman <[email protected]>: Attention is currently required from: Wail Alkowaileet, Vijay Sarathy, Glenn Galvizo. Ali Alsuliman 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 10: (29 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/42e766b7_833f27e6 PS10, Line 79: buildFulltextContainsRuleCollection() just asking the same question as Glenn's for the reason for moving this up. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/SqlppCompilationProvider.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/caaee54f_7902a493 PS10, Line 92: "cardinality" we should remove this. 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/ce50b617_350c7fed PS10, Line 364: accidentally added? 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/631b3d6d_b3e46f6f PS10, Line 500: accidentally added? 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/910c54fb_3e9c6e75 PS10, Line 369: joinEnumCode rename similar to the other one; checkApplicableOnly File asterixdb/asterix-app/src/test/resources/runtimets/queries_sqlpp/comparison/incomparable_types/incomparable_types.004.query.sqlpp: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/45c6ec11_b95f2ff6 PS10, Line 26: select data; revert as there is no change. File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/CompilerProperties.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/9cd650a3_4fd82b94 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: return accessor.getString(Option.COMPILER_CBO); then the caller deals with the value. File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/ae93bb70_8ec30462 PS10, Line 660: Pattern number = Pattern.compile("\\d+\\.\\d+"); : Pattern stringNumber = Pattern.compile("\\w+\\s+\\d+\\.\\d+"); : Pattern lessThanOnePat = Pattern.compile("0\\.\\d+"); not sure about the need for this, but we should be using this SQLPP parser itself to parse tokens like in ParenthesizedIdentifierList() and parseExpression() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/3396615a_4740725f PS10, Line 3086: SqlppHint.HASH_JOIN_HINT are we intending to add the hash join hint here? i.e. before a function call? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/20bdd40b_60268c5e PS10, Line 3590: while (hintToken != null) { : annotation = parseExpression i didn't quite get the intention of this change. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/5ca884d5_188d2650 PS10, Line 3643: SqlppHint.HASH_JOIN_HINT are we intending to add the hash join hint to the BETWEEN operator? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/98efc4c7_cd042092 PS10, Line 3736: fetchHint(token, SqlppHint.SINGLE_DATASET_PREDICATE_SELECTIVITY_HINT); we should add a test case for this (where we also test that only this hint is applicable in LIKE) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/8853a989_9db7a8f8 PS10, Line 5408: // make this a while loop same thing, i didn't quite get this. File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/base/ILogicalExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/f039ee36_c4825090 PS10, Line 42: substituteVarOnce we should revert this file once we move the logic as in my other comment. 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/64d8418d_65866bae 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 layer, as well, to OperatorAnnotation.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/AbstractFunctionCallExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/828c1cb9_75e58737 PS10, Line 267: setAnnotation AbstractFunctionCallExpression is kind of generic, and the implementation here seems to be very specific to a use case. we should move this logic to where we want it; to EnumerateJoinsRule File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/expressions/VariableReferenceExpression.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/5342f5c7_0b6cbb08 PS10, Line 85: MutableBoolean changed it does not feel right to have an external boolean control the logic of this public method, especially being an interface method. i checked how this method is used. we should just move the logic to EnumerateJoinsRule. 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/cec6a29f_6a11699b PS10, Line 69: EstimatedCostComputationVisitor we should move this to asterixdb layer (cbo package) so that it's with AnnotateOperatorCostCardinalityRule where it's used. File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/c7f53aeb_8a8c95a4 PS10, Line 129: /* : for (Map.Entry<String, Object> anno : op.getAnnotations().entrySet()) { remove commented code (you can just roll back the whole file) File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitorJson.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/904d40f2_efdb049a PS10, Line 215: equals you can use == for tags, same thing for the one below it. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/924a58b1_0dd9bfdf PS10, Line 237: revert adding extra line File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/util/OperatorManipulationUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/a67900fe_7e91d227 PS10, Line 316: op.getInputs()) > That is not a copy and it still points to the original input list? Is that > intended? it feels like OperatorManipulationUtil.bottomUpCopyOperators() should be used. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/6c5d74ff_36e9f311 PS10, Line 527: //ScalarFunctionCallExpression scalarExpr = (ScalarFunctionCallExpression) joinExpr; remove commented line. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/1a80a6f3_bd747276 PS10, Line 529: if (arg.getValue().getExpressionTag().equals(LogicalExpressionTag.FUNCTION_CALL)) { we can use == for tags comparison: for (Mutable<ILogicalExpression> argRef : inExpr.getArguments()) { ILogicalExpression arg = argRef.getValue(); if (arg.getExpressionTag() == LogicalExpressionTag.FUNCTION_CALL) { replaceVarWithExpr((AbstractFunctionCallExpression) arg, var, replacementExpr); } else if (arg.getExpressionTag() == LogicalExpressionTag.VARIABLE) { LogicalVariable v = ((VariableReferenceExpression) arg).getVariableReference(); if (v.equals(var)) { argRef.setValue(replacementExpr.cloneExpression()); } } } https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/71df383c_45c71688 PS10, Line 534: // cloning is needed according to Dmitry. remove comment. 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/4db361a3_b5c4d925 PS10, Line 55: public static double selectivityForSecondaryIndexSelection = 0.1; we should move this to a better place. move it to Stats.java for now. make it uppercase SELECTIVITY_FOR_SECONDARY_INDEX_SELECTION https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/f5470e2b_3df61d34 PS10, Line 251: // default value if option is not in ("on", "off", "test") we can remove these comments since the code is clear about it https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/c5e3aeff_c61e3adb 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 https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/59aba371_686538ec 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. also, i'm wondering if the values of these modes should be case-sensitive or not. need to look at existing properties. -- 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: 10 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: Vijay Sarathy <[email protected]> Gerrit-Attention: Glenn Galvizo <[email protected]> Gerrit-Comment-Date: Tue, 13 Sep 2022 05:18:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Wail Alkowaileet <[email protected]> Gerrit-MessageType: comment
