Dmitry Lychagin has posted comments on this change.

Change subject: [ASTERIXDB-2344] LIMIT pushdown for primary index
......................................................................


Patch Set 9:

(11 comments)

https://asterix-gerrit.ics.uci.edu/#/c/2541/9/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/PushLimitIntoPrimaryRule.java
File 
asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/PushLimitIntoPrimaryRule.java:

Line 46: public class PushLimitIntoPrimaryRule implements IAlgebraicRewriteRule 
{
Can we rename it into PushLimitIntoPrimaryIndexSearchRule ?


Line 68:         if (childOp.getValue().getOperatorTag() == 
LogicalOperatorTag.EXCHANGE) {
How come PushLimitIntoOrderByRule which runs before this rule does not consider 
EXCHANGE operators? Can you explain this? 
It seems that ONE_TO_ONE Exchange operators are introduced later by the 
RuleCollections.prepareForJobGenRuleCollection(). So if you see EXCHANGE at 
this point then it's not 1:1 and the LIMIT should not be pushed through it. Is 
this correct?


Line 79:             context.computeAndSetTypeEnvironmentForOperator(op);
use OperatorPropertiesUtil.typeOpRec(opRef, context) instead.


Line 85:         long outputLimit = 
AccessMethodUtils.getInt64Constant(limit.getMaxObjects());
first you need to check whether maxobject expression is a constant: 
limitOp.getMaxObjects().getValue().getExpressionTag() == 
LogicalExpressionTag.CONSTANT
because AccessMethodUtils.getInt64Constant() does not do that.


Line 86:         if (limit.getOffset() != null && limit.getOffset().getValue() 
!= null) {
you do need to check limit.getOffset().getValue() != null, but you do need to 
check that limit.getOffset() is a constant expression.


Line 136:         ILogicalExpression unnestExpr = 
op.getExpressionRef().getValue();
check that this op.getOutputLimit() is null, so nothing got pushed to it yet.


Line 150:         boolean isSource = op.getInputs().isEmpty()
also check that op.getOutpuLimit() is null, so nothing got pushed to it yet.


Line 152:         boolean isDataset = op.getDataSource() instanceof 
DatasetDataSource;
use ((DataSource)op.getDataSource()).getDatasourceType() == 
DataSource.Type.INTERNAL_DATASET instead of instanceof and the next line


https://asterix-gerrit.ics.uci.edu/#/c/2541/9/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java
File 
asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/MetadataProvider.java:

PS9, Line 1548: getTupleFilterFactory
can we rename it back to createTupleFilterFactory()? it creates a new instance 
so "create*" is better than "get*"


https://asterix-gerrit.ics.uci.edu/#/c/2541/9/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java
File 
hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java:

Line 238:             if (tupleFilter != null && 
!tupleFilter.accept(referenceFilterTuple.reset(tuple))) {
It seems that if tupleFilter expression uses any variables from the 
index-search's input then it'll no longer work because you're evaluating in on 
the tuple you get from the cursor which does not have those fields. You need 
address this problem in the optimizer rule you added. Check that all free 
variables of the select expression you're pushing into the index-search are 
produced by that index-search. 
Actually, it's not only variables from the operator input, but other values 
added to the output tuple by this method (appendSearchCallbackProceedResult, 
appendIndexFilter). Any of them can in theory be used by the pushed select 
operator.


Line 268:             FrameUtils.appendConcatToWriter(writer, appender, 
accessor, tupleIndex,
before outputting this missing tuple you need to check here that it:
1) satisfies the tupleFilter, and 
2) still within the limit (corner case but LIMIT 0 is possible)


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/2541
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I824fcad79995325e12a1a81d629160025294b915
Gerrit-PatchSet: 9
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Luo Chen <cl...@uci.edu>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Dmitry Lychagin <dmitry.lycha...@couchbase.com>
Gerrit-Reviewer: Ian Maxon <ima...@apache.org>
Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Luo Chen <cl...@uci.edu>
Gerrit-Reviewer: Taewoo Kim (please use wangs...@gmail.com) <taew...@uci.edu>
Gerrit-Reviewer: Taewoo Kim <wangs...@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to