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 <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Luo Chen <[email protected]> Gerrit-Reviewer: Taewoo Kim (please use [email protected]) <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
