>From Wail Alkowaileet <[email protected]>: Wail Alkowaileet 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 1: (21 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/compiler/provider/SqlppCompilationProvider.java@93 PS1, Line 93: cardinality Probably should be declared as a constant -- similar to "COMPILER_FORCEJOINORDER_KEY" https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/Cost.java@22 PS1, Line 22: ICost Could be comparable (i.e., extending Comparable) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/Cost.java@24 PS1, Line 24: cost It seems "cost" could be final https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/ICost.java@22 PS1, Line 22: ICost Maybe "extends Comparable<ICost>"? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@86 PS1, 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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@127 PS1, Line 127: OnlyOneAssign "onlyOneAssign" https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@128 PS1, Line 128: equals == could be used instead of equals() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@134 PS1, Line 134: equals == could be used instead of equals() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@304 PS1, Line 304: equals == could be used instead of equals() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/EnumerateJoinsRule.java@312 PS1, Line 312: printPlan 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/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinCondition.java@20 PS1, 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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@76 PS1, Line 76: JoinEnum Use Map<> instead of HashMap<>. This occurred in several place (e.g., line#111) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@203 PS1, Line 203: HashSet Set<> instead of HashSet<> https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinEnum.java@203 PS1, Line 203: HashSet<LogicalVariable> vars = new HashSet<>(); Declare outside the loop and use clear instead of creating a new one each time. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@247 PS1, Line 247: IntroduceJoinAccessMethodRule tmp = new 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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/JoinNode.java@388 PS1, Line 388: IntroduceSelectAccessMethodRule tmp = new IntroduceSelectAccessMethodRule() Same here for creating/including a rule here https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/PlanNode.java@31 PS1, Line 31: static Remove static. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/StatsUtil.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/StatsUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/util/StatsUtil.java@39 PS1, Line 39: StatsUtil Rename it to Stats or CBOStats instead of StatsUtil. Usually "util" classes cannot be instantiated. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/config/AlgebricksConfig.java@38 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". To be consistent with other flags https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/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/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/PhysicalOptimizationConfig.java@248 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. 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 to the test mode. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17143/1/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/rewriter/base/PhysicalOptimizationConfig.java@304 PS1, 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/+/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: 1 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-Comment-Date: Fri, 19 Aug 2022 00:11:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
