>From Vijay Sarathy <[email protected]>: Attention is currently required from: Ali Alsuliman. Vijay Sarathy has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301 )
Change subject: [ASTERIXDB-3094][COMP] Add cost and cardinality to explain ...................................................................... Patch Set 6: (23 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/915785b3_f3472971 PS4, Line 546: AbstractFunctionCallExpression afce = (AbstractFunctionCallExpression) expr; : if (afce.getFunctionIdentifier().equals(AlgebricksBuiltinFunctions.EQ)) : return true; > this is causing class cast exception in many tests (e.g. […] I only put this in for the demo as a quick fix for Q9. Will transfer this back to Murali after the S&T demo. I will back it out of this patch, this has no connection to this patch. File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/8a3e712b_5b7e63b2 PS4, Line 331: double cost = : (double) plan.getRoots().get(0).getValue().getAnnotations().get(OperatorAnnotations.OP_COST_TOTAL); : double cardinality = (double) plan.getRoots().get(0).getValue().getAnnotations() : .get(OperatorAnnotations.OP_OUTPUT_CARDINALITY); > we may not necessarily have these annotations. we could get null here. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/68437caf_f11211a1 PS4, Line 330: Pair<Double, Double> cardCost = new Pair<>(0.0, 0.0); : double cost = : (double) plan.getRoots().get(0).getValue().getAnnotations().get(OperatorAnnotations.OP_COST_TOTAL); : double cardinality = (double) plan.getRoots().get(0).getValue().getAnnotations() : .get(OperatorAnnotations.OP_OUTPUT_CARDINALITY); : cardCost.setFirst(cardinality); : cardCost.setSecond(cost); > take this out into a separate method. Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/ea109220_42917063 PS5, Line 521: isExplainOnly > rename to "printCostEstimates" Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/591bceb2_f6c1db4f PS5, Line 527: isExplainOnly > same Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/fccbdca2_d20c10d5 PS5, Line 532: isExplainOnly > same Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/fields/ExplainOnlyResultsPrinter.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/1469cb00_13c93c98 PS4, Line 36: public enum ExplainOnlyResults { : CARDINALITY("cardinality"), : COST("cost"), : PLAN("plan"); : : private final String str; : : ExplainOnlyResults(String str) { : this.str = str; : } : : public String str() { : return str; : } : } > we could just use plain String variables. Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/31874fd4_f3cc2f4c PS4, Line 64: pw.print("\t\""); : pw.print(AbstractResultsPrinter.FIELD_NAME); : pw.print("\": ["); : pw.print("\n\t{\n\t"); : ResultUtil.printField(pw, ExplainOnlyResults.CARDINALITY.str(), explainResults.getCardinality()); : pw.print("\n\t"); : ResultUtil.printField(pw, ExplainOnlyResults.COST.str(), explainResults.getCost()); : pw.print("\n\t"); : ResultUtil.printFieldNoQuotes(pw, ExplainOnlyResults.PLAN.str(), : explainResults.getPlan().replaceAll("\n", "\n\t\t"), false); : //ResultUtil.printResults(appCtx, explainResults.getPlan(), sessionOutput, new IStatementExecutor.Stats(), null); : pw.print("\n\t}"); : pw.print("\n]"); > this causes problems. […] Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/result/fields/ExplainResults.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/eb8a732e_357a5381 PS4, Line 21: ExplainResults > rename it to "ExplainResult" Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/715fe29f_b2b1c346 PS4, Line 22: private double cardinality; : private double cost; : private String plan; > make all final Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/IPlanPrettyPrinter.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/a4da54f0_c209818f PS5, Line 31: IPlanPrettyPrinter > maybe we should rename the boolean flag to say "printCostEstimates" Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/b2d2f0d5_ba5c8126 PS5, Line 34: default IPlanPrettyPrinter printOperator(AbstractLogicalOperator operator, boolean isExplainOnly) : throws AlgebricksException { : return printOperator(operator, isExplainOnly); : } > I don't think this is right. the meaning has changed now. […] Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/e89498c0_5920519d PS5, Line 94: isExplainOnly > rename all flags to say "printCostEstimates" Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/03fdb401_11858935 PS5, Line 100: public final IPlanPrettyPrinter printPlan(ILogicalPlan plan, Map<Object, String> log2phys, boolean isExplainOnly) > same Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/2b81543d_1e6b1d86 PS5, Line 109: boolean isExplainOnly) throws AlgebricksException { > same Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitorJson.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/7442af2c_4a69fa7d PS4, Line 247: optimizer_estimates > can we make it "optimizer-estimates"? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/c16c32d4_3037f5e5 PS4, Line 217: String opCard = null; : String opCostLocal = null; : String opCostTotal = null; : for (Map.Entry<String, Object> anno : op.getAnnotations().entrySet()) { : Object annotationVal = anno.getValue(); : if (annotationVal != null) { : String annotation = anno.getKey(); : switch (annotation) { : case OperatorAnnotations.OP_COST_LOCAL: : opCostLocal = annotationVal.toString(); : break; : case OperatorAnnotations.OP_COST_TOTAL: : opCostTotal = annotationVal.toString(); : break; : case OperatorAnnotations.OP_INPUT_CARDINALITY: : if (op.getOperatorTag() == LogicalOperatorTag.DATASOURCESCAN) { : opCard = annotationVal.toString(); : } : break; : case OperatorAnnotations.OP_OUTPUT_CARDINALITY: : if (op.getOperatorTag() != LogicalOperatorTag.DATASOURCESCAN) { : opCard = annotationVal.toString(); : } : break; : default: : break; : } : } : } : if (opCard != null && opCostLocal != null && opCostTotal != null) { : jsonGenerator.writeObjectFieldStart("optimizer_estimates"); : jsonGenerator.writeStringField(OP_CARDINALITY, opCard); : jsonGenerator.writeStringField(OperatorAnnotations.OP_COST_LOCAL.toLowerCase().replace('_', '-'), : opCostLocal); : jsonGenerator.writeStringField(OperatorAnnotations.OP_COST_TOTAL.toLowerCase().replace('_', '-'), : opCostTotal); : jsonGenerator.writeEndObject(); : } > it's better to extract all of this into a separate method for readability. Done File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/prettyprint/LogicalOperatorPrettyPrintVisitorJson.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/75e9ca44_50f40026 PS5, Line 162: public final IPlanPrettyPrinter printPlan(ILogicalPlan plan, boolean isExplainOnly) throws AlgebricksException { > same Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/b7247064_2246e859 PS5, Line 169: public final IPlanPrettyPrinter printPlan(ILogicalPlan plan, Map<Object, String> log2phys, boolean isExplainOnly) > same Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/712c878d_f70b2250 PS5, Line 178: public final IPlanPrettyPrinter printOperator(AbstractLogicalOperator op, boolean printInputs, boolean isExplainOnly) > same Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/2a5f58d1_94d99f96 PS5, Line 185: private void printPlanImpl(ILogicalPlan plan, boolean isExplainOnly) throws AlgebricksException { > same Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/67e8808b_87718b6d PS5, Line 202: private boolean nestPlanInPlanField(AbstractLogicalOperator op, boolean isExplainOnly) throws IOException { > rename flag to "printCostEstimates". […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/4c574230_259af54c PS5, Line 267: isExplainOnly > rename to "printCostEstimates" Done -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301 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: I7b4a6c78dca3326f070e1dda888d774dc6100c0b Gerrit-Change-Number: 17301 Gerrit-PatchSet: 6 Gerrit-Owner: Vijay Sarathy <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-CC: Ali Alsuliman <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Comment-Date: Thu, 15 Dec 2022 12:25:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Ali Alsuliman <[email protected]> Gerrit-MessageType: comment
