Steven Jacobs has posted comments on this change. Change subject: ASTERIXDB-1608, ASTERIXDB-1617 Match user query for nonpure function calls ......................................................................
Patch Set 18: (29 comments) I cleaned things up a lot from your comments and email, including the headers. The relationship with unpartitioned is as follows: The only way that we can use a non pure value in an index-search is if that value is global across all nodes. This will be true only in one case. The user will have a let statement that is not contained by any for statements, meaning that the user's intension is to compute a single value, not a value for every record. In this case the plan will include an unpartitioned assign that will be broadcast to all nodes. I'll push the fixes 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? It's not needed anymore actually. I removed it :) Line 567: > Why do we remove this check? fieldName == null? This is so we can still get and remember the type of the value. In this case it will be from an assign so it won't have a field name Line 663: expr = (AbstractLogicalExpression) assignOp.getExpressions().get(assignVarIndex).getValue(); > Why do we remove this check? Fixed 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 Done Line 101: typeEnvironment); > I think this kind of format changes that are not caused by your changes are Done Line 137: : subTree.getAssignsAndUnnestsRefs().get(0); > I think this kind of format changes that are not caused by your changes are Done Line 229: .getValue(); > I think this kind of format changes that are not caused by your changes are Done Line 275: indexSubTree, probeSubTree); > I think this kind of format changes that are not caused by your changes are Done Line 567: primaryIndexSearch, primaryIndexFuncArgs); > I think this kind of format changes that are not caused by your changes are Done Line 676: .getComparisonType(optFuncExpr.getFuncExpr().getFunctionIdentifier()); > I think this kind of format changes that are not caused by your changes are Done 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? Yes, I guess this comment is wrong. The second check fails in the self join case. I'll fix the comment 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 Done Line 856: throws AlgebricksException { > I think this kind of format changes that are not caused by your changes are Done 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? There may be non-pure ops that are passed (below) the scan. We treat these differently than the movable ops above the scan. Line 111: // Match (assign | unnest | exchange)+. > Can we have an exchange operator in this phase (logical transformation)? I think you're right. I needed this in one of the first patch sets, but not anymore. I'll take it out. Line 306: ds.getMetaItemTypeName()); > I think this kind of format changes that are not caused by your changes are Done Line 417: .get(idx).getValue(); > I think this kind of format changes that are not caused by your changes are Done 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 Done 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. Done 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)? isMovable includes both the annotation check and the non pure check 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 Done Line 199: if (op instanceof DataSourceScanOperator) { > op.getTypeTag() might be cheaper. Done Line 262: requiredProperty.getLocalProperties()); > I think this kind of format changes that are not caused by your changes are Done Line 284: mayExpandPartitioningProperties, context); > I think this kind of format changes that are not caused by your changes are Done Line 441: .getDeliveredPhysicalProperties(); > I think this kind of format changes that are not caused by your changes are Done Line 482: ? g.getPreferredOrderEnforcer() : g.getColumnSet(); > I think this kind of format changes that are not caused by your changes are Done Line 569: cldLocals); > I think this kind of format changes that are not caused by your changes are Done 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? We can't extract common expressions if they are non pure calls. They need to be maintained at the partitioning that they are currently at. 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? isMovable checks for both annotation and non pure -- 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
