Steven Jacobs has posted comments on this change. Change subject: ASTERIXDB-1608, ASTERIXDB-1617 Match user query for nonpure function calls ......................................................................
Patch Set 7: (7 comments) Hopefully I addressed at least some of the concerns. As I mentioned in email, there are some serious underlying issues with the access methods that make this change uglier than it needs to be. Maybe if you still have a lot of concerns, we can have a meeting to discuss? https://asterix-gerrit.ics.uci.edu/#/c/1057/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/PushFieldAccessRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/PushFieldAccessRule.java: Line 187: Object annotation = op2.getAnnotations().get("isMovable"); > call OperatorPropertiesUtil.isMovable(op2) instead? This is because I altered OperatorPropertiesUtil.isMovable to include a check on whether the operator is non pure. In the case of this rule however, it is okay to move the field access below the non pure call, since this doesn't alter the plan in a way that goes against the user's query. So we only specifically check whether the manual flag has been set. https://asterix-gerrit.ics.uci.edu/#/c/1057/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodUtils.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodUtils.java: Line 314: if (optFuncExpr.getNumConstantExpr() == 0) { > Can you explain a bit in the comments, why getNumConstantExpr() == 0 means I'll add this to the comments, but here is the summary: probeSubTree == null tells us that we are doing a select and not a join. In this case, the expression will either have a constant expr to compare to, or it will have two variables, where one variable is a reference to a nonPure call (i.e. numConstantExpr == 0) Line 370: return new Pair<ILogicalExpression, Boolean>(optFuncExpr.getConstantExpr(0), false); > please address this. okay https://asterix-gerrit.ics.uci.edu/#/c/1057/7/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 728: if (optFuncExpr.getFieldType(0) == null || optFuncExpr.getFieldType(1) == null) { > Comment why this is the non-pure case? I still couldn't understand the cond The previous code alone was already not 100% accurate. It says: "If my expression involved two variables, and there is an index nests-loop join hint, return true." It doesn't check whether it successfully found the source of one of the variables or not. I think this gets by because these checks are done elsewhere. https://asterix-gerrit.ics.uci.edu/#/c/1057/7/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 112: while ((subTreeOp.getOperatorTag() == LogicalOperatorTag.ASSIGN > pls address those. okay https://asterix-gerrit.ics.uci.edu/#/c/1057/7/asterixdb/asterix-app/src/test/resources/optimizerts/queries/nonpure/maintain-nonpure-location-in-join-cannot-index.aql File asterixdb/asterix-app/src/test/resources/optimizerts/queries/nonpure/maintain-nonpure-location-in-join-cannot-index.aql: Line 39: return { > Why this one cannot use index? This query doesn't pick up the index currently. I think it would be possible to do, but I never looked at optimizing the join case. It seems like an edge case to me. If this query were really desirable, it could be written in a way that makes sense logically and does use the index with my change, e.g. BETTER QUERY1: let $time := current-time()-time("123045678+0800") for $y in dataset Users2 where $y.stamp > $time for $x in dataset Users1 return { "x_id": $x.id, "y_id": $y.id } I can try with /*+ indexnl */ to see if it changes anything https://asterix-gerrit.ics.uci.edu/#/c/1057/7/asterixdb/asterix-app/src/test/resources/optimizerts/results/query_issue849.plan File asterixdb/asterix-app/src/test/resources/optimizerts/results/query_issue849.plan: Line 28: -- UNNEST |UNPARTITIONED| > ASTERIXDB-1368 is still open. But it seems your queries are able to use ind I wasn't aware of 1368. I'd have to know more about the underlying issues to know for sure whether it is fixed or not. -- 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: 7 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: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
