Taewoo Kim has posted comments on this change. Change subject: ASTERIXDB-1608, ASTERIXDB-1617 Match user query for nonpure function calls ......................................................................
Patch Set 18: (29 comments) 1) Let's not change the license header since the next change from another developer will revert this again. 2) The relationship between Unpartitioned_mode and non-pure function call: can you explain more? https://asterix-gerrit.ics.uci.edu/#/c/1057/18/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AbstractIntroduceAccessMethodRule.java: Line 228: if (j != exprAndVarIdx.second && optFuncExpr.getFieldType(j) != null) { Why this [&& optFuncExpr.getFieldType(j) != null] needs to be augmented? Line 567: Why do we remove this check? fieldName == null? Line 663: expr = (AbstractLogicalExpression) assignOp.getExpressions().get(assignVarIndex).getValue(); Why do we remove this check? https://asterix-gerrit.ics.uci.edu/#/c/1057/18/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/BTreeAccessMethod.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/BTreeAccessMethod.java: Line 86: AlgebricksBuiltinFunctions.GE, AlgebricksBuiltinFunctions.LT, AlgebricksBuiltinFunctions.GT)); I think this kind of format changes that are not caused by your changes are not necessary. Line 101: typeEnvironment); I think this kind of format changes that are not caused by your changes are not necessary. Line 137: : subTree.getAssignsAndUnnestsRefs().get(0); I think this kind of format changes that are not caused by your changes are not necessary. Line 229: .getValue(); I think this kind of format changes that are not caused by your changes are not necessary. Line 275: indexSubTree, probeSubTree); I think this kind of format changes that are not caused by your changes are not necessary. Line 567: primaryIndexSearch, primaryIndexFuncArgs); I think this kind of format changes that are not caused by your changes are not necessary. Line 676: .getComparisonType(optFuncExpr.getFuncExpr().getFunctionIdentifier()); I think this kind of format changes that are not caused by your changes are not necessary. Line 736: //We are in the select case (trees are the same) How about self-join case? Aren't two branches the same for this case? https://asterix-gerrit.ics.uci.edu/#/c/1057/18/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/InvertedIndexAccessMethod.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/InvertedIndexAccessMethod.java: Line 147: throws AlgebricksException { I think this kind of format changes that are not caused by your changes are not necessary. Line 856: throws AlgebricksException { I think this kind of format changes that are not caused by your changes are not necessary. https://asterix-gerrit.ics.uci.edu/#/c/1057/18/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java: Line 89: boolean passedSource = false; What does this variable do? Line 111: // Match (assign | unnest | exchange)+. Can we have an exchange operator in this phase (logical transformation)? Line 306: ds.getMetaItemTypeName()); I think this kind of format changes that are not caused by your changes are not necessary. Line 417: .get(idx).getValue(); I think this kind of format changes that are not caused by your changes are not necessary. https://asterix-gerrit.ics.uci.edu/#/c/1057/18/asterixdb/asterix-app/src/test/resources/optimizerts/queries/nonpure/global-datetime-no-index.aql File asterixdb/asterix-app/src/test/resources/optimizerts/queries/nonpure/global-datetime-no-index.aql: Line 19: Can we put some description here? What this test intends for? For all test cases? https://asterix-gerrit.ics.uci.edu/#/c/1057/18/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/util/OperatorPropertiesUtil.java File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/util/OperatorPropertiesUtil.java: Line 292: if (op instanceof AssignOperator) { op.getTypeTag() might be cheaper. https://asterix-gerrit.ics.uci.edu/#/c/1057/18/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ConsolidateAssignsRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ConsolidateAssignsRule.java: Line 57: if (!OperatorPropertiesUtil.isMovable(op) || !OperatorPropertiesUtil.isMovable(op2)) { Here, why we don't use annotation (your change)? https://asterix-gerrit.ics.uci.edu/#/c/1057/18/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/EnforceStructuralPropertiesRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/EnforceStructuralPropertiesRule.java: Line 131: new LinkedList<ILocalStructuralProperty>()); I think this kind of format changes that are not caused by your changes are not necessary. Line 199: if (op instanceof DataSourceScanOperator) { op.getTypeTag() might be cheaper. Line 262: requiredProperty.getLocalProperties()); I think this kind of format changes that are not caused by your changes are not necessary. Line 284: mayExpandPartitioningProperties, context); I think this kind of format changes that are not caused by your changes are not necessary. Line 441: .getDeliveredPhysicalProperties(); I think this kind of format changes that are not caused by your changes are not necessary. Line 482: ? g.getPreferredOrderEnforcer() : g.getColumnSet(); I think this kind of format changes that are not caused by your changes are not necessary. Line 569: cldLocals); I think this kind of format changes that are not caused by your changes are not necessary. https://asterix-gerrit.ics.uci.edu/#/c/1057/18/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ExtractCommonExpressionsRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/ExtractCommonExpressionsRule.java: Line 246: if (expr.isFunctional()) { The reason of this check? https://asterix-gerrit.ics.uci.edu/#/c/1057/18/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/PushMapOperatorDownThroughProductRule.java File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/PushMapOperatorDownThroughProductRule.java: Line 54: if (!OperatorPropertiesUtil.isMovable(op1)) { Again, why we don't use your change? -- To view, visit https://asterix-gerrit.ics.uci.edu/1057 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2dec322b30835625430c06acd7626d902bada137 Gerrit-PatchSet: 18 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Steven Jacobs <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Preston Carman <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
