>From Vijay Sarathy <[email protected]>: Attention is currently required from: Ian Maxon, Till Westmann, [email protected]. Vijay Sarathy has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765 )
Change subject: [ASTERIXDB-3246][COMP]: CBO costing of all physical operators in a query plan ...................................................................... Patch Set 19: Code-Review+1 (16 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/CostMethods.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/0c4fe502_8677e494 PS17, Line 184: ; > Should this return 0 or some kind of singleton "uncosted" cost to avoid > passing null around? Removed costLimit() as it is not used currently. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/fed79954_434bfb38 PS17, Line 192: cardCost.setFirst((Double) anno.getValue()); > What happens if one or both of the annotations are null? Is 0. […] Yes. 0.0 is always safe. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/5e6bc522_680882d0 PS17, Line 91: HashMap > this should be private, probably? Done. Changed other members to private too. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/607781bc_575f9c42 PS17, Line 94: private ILogicalOperator rootGroupByDistinctOp; : : // The OrderBy operator at root of the query tree (if exists) : private ILogicalOperator rootOrderByOp; : > i don't see why these need to be members of the class, can't they just be > returned from the methods […] This is needed because rewritePre() is called several times and we don't want to overwrite this on subsequent calls. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/ec2dc3a2_b23112bb PS17, Line 443: op = op.getInputs().get(0).getValue(); > don't reuse the reference in the method arguments for this Done. Changed many other routines that had similar "bad" reuse of arguments. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/32f07748_c099c59f PS17, Line 464: op = op.getInputs().get(0).getValue(); > again, use a local variable instead of reusing the method argument for this Done. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/EstimatedCostComputationVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/a929ae08_68cd0fc7 PS17, Line 282: : double distinctCost = 0.0; : Pair<Double, Double> cardCost = op.getInputs().get(0).getValue().accept(this, arg); : : for (Map.Entry<String, Object> anno : op.getAnnotations().entrySet()) { : if (anno.getValue() != null && anno.getKey().equals(OperatorAnnotations.OP_OUTPUT_CARDINALITY)) { : cardCost.setFirst((Double) anno.getValue()); : } : if (anno.getValue() != null && anno.getKey().equals(OperatorAnnotations.OP_COST_LOCAL)) { : distinctCost = (double) anno.getValue(); : } : } : double totalCost = cardCost.getSecond() + distinctCost; : op.getAnnotations().put(OperatorAnnotations.OP_COST_TOTAL, (double) Math.round(totalCost * 100) / 100); : cardCost.setSecond(totalCost); : : return cardCost; > this seems really similar to groupby cost, is there some way to share it? Done. Abstracted into a new common method annotateGroupByDistinct() File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/OperatorUtils.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/4d906fee_7cf59c09 PS17, Line 51: public static boolean containsAllGroupByDistinctVarsInScanOp(DataSourceScanOperator scanOp, > this isn't used, is it used in an extension? should it be moved there if so, > or is there a plan to u […] Removed all unused methods. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/12b9d21c_81c39aa3 PS17, Line 77: IOptimizationContext context, HashMap<DataSourceScanOperator, ILogicalOperator> map) { > what is this map supposed to contain? and why not use Map<> instead of a > particular implementation? Changed to use Map<> https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/427743f1_1206f48e PS17, Line 161: return new Pair<>(distinctVars, distinctFunctions); > you could just instantiate a pair before this with these and return it in > both the null/identity cas […] Done. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/9d44439d_789eea85 PS17, Line 173: for (int i = 0; i < argList.size(); i += 2) { > is argList's size always even? Yes. This is true for the argument list of AbstractFunctionCallExpression and only odd position arguments are field value expressions. Added a comment to the code to make it clearer. There are several other places in the code base with similar code. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/Stats.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/b7a63ab2_30acd3af PS17, Line 585: String viewInPlan = new ALogicalPlanImpl(new MutableObject<>(grpByDistinctOp)).toString(); //useful when debugging > should probably wrap this in LOGGER. […] Removed this code, not needed here. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/e05b7e85_6ef05998 PS17, Line 635: totalSamples > what if totalSamples isn't set? is there some safe default value that can be > explicitly set when it' […] This is a little tricky to do the way Mehnaz wrote the code. I have abstracted the code into a new function initNR() to initialize totalSamples and added comments to make it more understandable. Did many other cleanups with more descriptive variable names in the process to make the code easier to understand, https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/975c5106_19dc4446 PS17, Line 696: op = op.getInputs().get(0).getValue(); > don't reuse the argument Done. File asterixdb/asterix-app/src/main/resources/cc.conf: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/aa00d3b0_58d0ce60 PS17, Line 56: compiler.groupmemory=32MB > why change this here? Reverted all such changes. File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/SetAlgebricksPhysicalOperatorsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/51a530ed_fea9ea5b PS17, Line 212: protected void addAnnotations(DistinctOperator distinct) { : } : > why's this a no-op? Modified the code to remove the no-op and moved the method to only be in SetAsterixPhysicalOperatorsRule as it is only needed there. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765 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: I3196f664d716bb5b3806ec9a5a0dd5c1ea51ff62 Gerrit-Change-Number: 17765 Gerrit-PatchSet: 19 Gerrit-Owner: Vijay Sarathy <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Vijay Sarathy <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-CC: Ian Maxon <[email protected]> Gerrit-Attention: Ian Maxon <[email protected]> Gerrit-Attention: Till Westmann <[email protected]> Gerrit-Attention: [email protected] Gerrit-Comment-Date: Sun, 29 Oct 2023 17:33:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Ian Maxon <[email protected]> Gerrit-MessageType: comment
