>From Ali Alsuliman <[email protected]>: Attention is currently required from: Vijay Sarathy. Ali Alsuliman 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 4: (9 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/3a9c902c_624eb6c7 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. casting from ConstantExpression to AbstractFunctionCallExpression). "expr" type is ILogicalExpression and can be anything like ConstantExpression and VariableReferenceExpression in addition to AbstractFunctionCallExpression File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/1b799517_7e5e8253 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. initialize them to 0 in that case. it's also better to check that getRoots() is not empty. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/ce9570aa_bd973827 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. 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/d6179a65_0e7f8346 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. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/5be0f4b7_c56aee0f 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. i don't think we should do this by hand here since we need to respect the settings in "sessionOutput" which can do some pre-printing and post-printing in addition to other things while printing. it's actually the cause of some test failures since "quoteResult" is not picked up which escapes and wraps the "string" plan in "", e.g.: "distribute result [$$2].....". "sessionOutput" does other things too like starting the "results" with a [ 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/e4198d44_68bc7752 PS4, Line 21: ExplainResults rename it to "ExplainResult" https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/6eabb2da_1e2ef3f6 PS4, Line 22: private double cardinality; : private double cost; : private String plan; make all final 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/5f7cea32_5147d373 PS4, Line 247: optimizer_estimates can we make it "optimizer-estimates"? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17301/comment/1035fd93_9198d7e9 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. -- 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: 4 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: Vijay Sarathy <[email protected]> Gerrit-Comment-Date: Wed, 14 Dec 2022 04:35:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
