>From Ian Maxon <[email protected]>: Attention is currently required from: Till Westmann, Vijay Sarathy, [email protected]. Ian Maxon 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 17: (17 comments) Patchset: PS17: could there be some tests added for this? File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/CostMethods.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/26ecb5e9_aa4f15d4 PS17, Line 184: ; Should this return 0 or some kind of singleton "uncosted" cost to avoid passing null around? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/5c54688a_ad342067 PS17, Line 192: cardCost.setFirst((Double) anno.getValue()); What happens if one or both of the annotations are null? Is 0.0 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/a60d1741_625346e0 PS17, Line 91: HashMap this should be private, probably? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/e439b23d_10f53413 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 that alter them respectively and kept as local variables? is there some use of them outside of the context of rewritePre()? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/87f566c9_8977c216 PS17, Line 443: op = op.getInputs().get(0).getValue(); don't reuse the reference in the method arguments for this https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/bee9dfc8_78f339b7 PS17, Line 464: op = op.getInputs().get(0).getValue(); again, use a local variable instead of reusing the method argument for this 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/7d6a8468_7dff8144 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? 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/a260809f_53f75f83 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 use it in the near future? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/8587a43e_7531b760 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? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/0b809de2_b50b8728 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 case and in the case there's actually something to return since the thing that actually changes is the arrays https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/0ad8bbee_127f9108 PS17, Line 173: for (int i = 0; i < argList.size(); i += 2) { is argList's size always even? 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/f83f81b4_eaad4a18 PS17, Line 585: String viewInPlan = new ALogicalPlanImpl(new MutableObject<>(grpByDistinctOp)).toString(); //useful when debugging should probably wrap this in LOGGER.isTraceEnabled() or whatever to not make it if it isn't going to be logged anyway https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/77361577_8385c24d PS17, Line 635: totalSamples what if totalSamples isn't set? is there some safe default value that can be explicitly set when it's declared? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/76656c93_aaaff7a7 PS17, Line 696: op = op.getInputs().get(0).getValue(); don't reuse the argument File asterixdb/asterix-app/src/main/resources/cc.conf: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17765/comment/b5fbc251_ce98c4ac PS17, Line 56: compiler.groupmemory=32MB why change this here? 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/b4bbb2ff_9c30d33d PS17, Line 212: protected void addAnnotations(DistinctOperator distinct) { : } : why's this a no-op? -- 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: 17 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: Till Westmann <[email protected]> Gerrit-Attention: Vijay Sarathy <[email protected]> Gerrit-Attention: [email protected] Gerrit-Comment-Date: Wed, 25 Oct 2023 23:28:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
