>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/+/17299 )
Change subject: [ASTERIXDB-XXXX][COMP] Misc. enhancements/fixes after calibration tests. ...................................................................... Patch Set 4: (9 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/cost/CostMethods.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299/comment/b0908ce1_981ca056 PS4, Line 40: /* 32 MB */ * 1024 / 8; // 4 GB; I'm not sure I followed this part. File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/cbo/EnumerateJoinsRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299/comment/f780d8d1_d7658edb PS4, Line 232: AssignOperator aOp = (AssignOperator) nextOp; : List<LogicalVariable> vars = new ArrayList<>(); : aOp.getExpressions().get(0).getValue().getUsedVariables(vars); : //if (vars.size() > 1) // This is to avoid the case where the vars do not belong to the same leaf Input. This is too restrictive but safe : //return false; I don't see this is being used. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299/comment/c6d77378_6cc3fb2c PS4, Line 242: LogicalOperatorTag tag; : while (true) { : if (op.getInputs().size() > 1) : return null; // Assuming only a linear plan for single table queries (as leafInputs are linear). : tag = op.getOperatorTag(); : if (tag == LogicalOperatorTag.EMPTYTUPLESOURCE) : return null; // if this happens, there is nothing we can do in CBO code since there is no datasourcescan : //if ((tag == LogicalOperatorTag.SELECT) || (tag == LogicalOperatorTag.DATASOURCESCAN)) { : if (tag == LogicalOperatorTag.SELECT) { // there must be a select operator for CBO to do any optimization. : return op; : } : : op = op.getInputs().get(0).getValue(); : } can we clean this up? remove commented out code (and probably rename the method since it's only looking for SELECT) and enclose if-blocks in {}. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299/comment/3e6578c0_89fdebbd PS4, Line 294: findSelectOrDataScan we just called findSelectOrDataScan() up above. can't we use the returned value? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299/comment/3078939b_9cacddce PS4, Line 388: i++; why do we have another "i++" at the bottom of the loop as well? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299/comment/90131b43_16900c24 PS4, Line 409: root = aOp; not sure why we need this assignment (and the root variable), but we could just return aOp https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299/comment/d1c71712_b219dda3 PS4, Line 507: if (internalEdges.size() == 0) : return op; enclose in {} https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299/comment/e8c64b1d_7d72b16f PS4, Line 634: HashSet<LogicalVariable> vars = new HashSet<>(); take this line out before the for-loop and clear the set here for re-use. (also we usually do it like this: Set<LogicalVariable> vars = new HashSet<>();) by the way, where is this "vars" used? File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299/comment/4afe8f02_83614fb2 PS4, Line 3674: if (hintToken != null) { : SourceLocation sourceLoc = getSourceLocation(hintToken); : hintCollector.remove(sourceLoc); : } I think what should have been done is to have another fetchHints() that returns a list of hintToken instead of fetchHint() that returns a single hint. fetchHints() will take care of checking the hints, removing them and issuing a warning for each one that is not from the expected list of hints. we can do that in a follow-up change. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17299 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: Ifa7da177ab94ba39ee2d3e24813eced48e3b8304 Gerrit-Change-Number: 17299 Gerrit-PatchSet: 4 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: Vijay Sarathy <[email protected]> Gerrit-CC: Till Westmann <[email protected]> Gerrit-CC: [email protected] Gerrit-Attention: Vijay Sarathy <[email protected]> Gerrit-Comment-Date: Mon, 05 Dec 2022 05:38:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
