asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steven Jacobs (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: ASTERIXDB-1608, ASTERIXDB-1617 Match user query for nonpure ...
Date Thu, 20 Oct 2016 21:43:52 GMT
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 <sjaco002@ucr.edu>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Preston Carman <prestonc@apache.org>
Gerrit-Reviewer: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-Reviewer: Taewoo Kim <wangsaeu@yahoo.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message