>From Wail Alkowaileet <wael....@gmail.com>: Wail Alkowaileet has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604 )
Change subject: [ASTERIXDB-3046][COMP] Changes to support cost based query optimization in Asterix DB. ...................................................................... Patch Set 45: (18 comments) First pass https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/SqlppCompilationProvider.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/SqlppCompilationProvider.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/SqlppCompilationProvider.java@93 PS45, Line 93: cardinality Probably should be declared as a constant -- similar to "COMPILER_FORCEJOINORDER_KEY" https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/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/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/Cost.java@22 PS45, Line 22: Cost Could be comparable (i.e., extending Comparable) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/Cost.java@24 PS45, Line 24: cost It seems "cost" could be final https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/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/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/ICost.java@22 PS45, Line 22: ICost Maybe "extends Comparable<ICost>"? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/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/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@86 PS45, Line 86: /* : MetadataProvider metadataProvider = (MetadataProvider) context.getMetadataProvider(); : ICcApplicationContext ccAppContext = metadataProvider.getApplicationContext(); : CompilerProperties compilerProperties = ccAppContext.getCompilerProperties(); : return compilerProperties.isCBOEnabled(); : * remove https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@127 PS45, Line 127: OnlyOneAssign "onlyOneAssign" https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@128 PS45, Line 128: equals == could be used instead of equals() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@134 PS45, Line 134: equals == could be used instead of equals() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@304 PS45, Line 304: equals == https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@314 PS45, Line 314: System.out.println("---------------------------- " + text); : pp.reset(); : pp.printOperator(op, true); : System.out.println(pp); : System.out.println("---------------------------- "); Should use Logger instead of System.out There's a plan printer for each fired rule. Is this necessary? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/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/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinCondition.java@20 PS45, Line 20: util Let's have EnumerateJoinsRule, JoinCondition, JoinNode, JoinEnum, PlanNode, and StatsUtil under a new package called "cbo". I.e., the package name would be "package org.apache.asterix.optimizer.rule.cbo" https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/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/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@247 PS45, Line 247: IntroduceJoinAccessMethodRule I'm not sure about creating/including a rule here at this stage? Ali and Glenn probably have a better take on this. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@388 PS45, Line 388: tmp Same here for creating/including a rule here https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/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/+/15604/45/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/PlanNode.java@31 PS45, Line 31: joinEnum This won't be thread-safe. Is that intended? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/config/AlgebricksConfig.java 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/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/config/AlgebricksConfig.java@38 PS45, Line 38: on could that be true/false instead of "on"/"off". To be consistent with other flags https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/config/AlgebricksConfig.java@39 PS45, Line 39: off same as above https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/PhysicalOptimizationConfig.java 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/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/PhysicalOptimizationConfig.java@248 PS45, Line 248: test Make cbo to be a boolean instead to be consistent with other flags. The "test" mode could have its own flag. I believe/think that the "test" mode should not be exposed to end-users. Thus, having it as a different flag could allow us to prevent the user from setting CBO ont the test mode. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604/45/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/PhysicalOptimizationConfig.java@304 PS45, Line 304: private void setString(String property, String value) { : properties.setProperty(property, value); : } : : private String getString(String property, String defaultValue) { : String value = properties.getProperty(property); : return value == null ? defaultValue : value; : } This shouldn't be needed if we use true/false -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/15604 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: I8d5fe4947df486d054fdf0f178fccf0c95eacfb6 Gerrit-Change-Number: 15604 Gerrit-PatchSet: 45 Gerrit-Owner: Vijay Sarathy <vijay.sara...@couchbase.com> Gerrit-Reviewer: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Glenn Galvizo <ggalv...@uci.edu> Gerrit-Reviewer: Ian Maxon <ima...@uci.edu> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Wail Alkowaileet <wael....@gmail.com> Gerrit-Reviewer: murali4...@gmail.com Gerrit-CC: Till Westmann <t...@couchbase.com> Gerrit-Comment-Date: Tue, 09 Aug 2022 17:57:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
