>From Ali Alsuliman <[email protected]>: Attention is currently required from: Vijay Sarathy. 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 7: (13 comments) Patchset: PS7: second batch of comments. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/SqlppCompilationProvider.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/dba73dad_967581fa PS6, Line 77: remove this extra line. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/58717957_4d71708e PS6, Line 356: SqlppHint.HASH_JOIN_HINT.getIdentifier() why are we passing the hint identifier (which is "hashjoin")? 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/075b3311_a8fa980f PS6, Line 210: chooseAllIndexes this change could lead to unintended consequences. see my other comment in IntroduceSelectAccessMethodRule File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/BTreeAccessMethod.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/d8c0d633_940f9f21 PS6, Line 1037: !joinEnumCode i'm a bit confused about this. also, the boolean flag needs a better name to reflect what it's controlling. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/BTreeAccessMethod.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/26903f83_786d4dfc PS7, Line 1045: remove extra line. 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/b5c0aed1_7dad53ed PS6, Line 472: chooseAllIndexes(analyzedAMs, chosenIndexes); i think this change could lead to some problems since now this change means something different than the original logic. "checkAndApplyTheSelectTransformation" is called recursively with the same "chosenIndexes" object which can accumulate indexes from previous invocations of the recursive method. the below if-block will continue execution since chosenIndexes is not empty, while the old logic gets and checks against a fresh list of chosenIndexes for each recursive call. 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/ae404435_cf87cc8f PS6, Line 807: CompilerProperties.COMPILER_FORCE_JOIN_ORDER_KEY are we planning to make this property configurable by user at the application level so that the user can change the compiler default? the reason i'm asking is because currently it's accessed through the metadataProvider which only sees the parameters configured via SET statements which means if the user actually wants all their queries to run with force.join.order set to "old", they will have to always issue a SET statement together with the query. Same thing goes with the plan shape property. File asterixdb/asterix-app/src/main/resources/cc2.conf: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/71e89f6d_7fe84a1b PS7, Line 52: compiler.queryplanshape=zigzag i don't think this is picked up. see my comment in EnumerateJoinsRule. File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/CompilerProperties.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/cdb4953e_ae557cef PS6, Line 283: public boolean isCBOEnabled() { : String cbo = getCBO(); : if (cbo.equals("on") || cbo.equals("test")) { : return true; : } : return false; : } remove this method since CompilerProperties normally just returns whatever the value of the property is, and the caller of the method takes care of using it. this method is also not used. File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/1cb56a2f_dc19ac92 PS6, Line 222: import org.apache.hyracks.algebricks.core.algebra.expressions.JoinProductivityAnnotation; : import org.apache.hyracks.algebricks.core.algebra.expressions.PredicateCardinalityAnnotation; : import org.apache.hyracks.algebricks.core.algebra.expressions.HashJoinExpressionAnnotation; we need to sort the imports alphapatically to keep the formatting happy. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/comment/3a58764c_01d9cb8b PS6, Line 706: HASH_JOIN_HINT how is this hint going to be used (because i see conflicting usages of the hint)? we should add test cases (optimizer + runtime when applicable) for each new hint we add. 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/75096e47_4501f049 PS6, Line 24: String buildAlias = null; i don't see this being used. -- 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: Vijay Sarathy <[email protected]> Gerrit-Comment-Date: Thu, 08 Sep 2022 02:30:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
