asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Young-Seok Kim (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Index-only plan
Date Sat, 09 Jul 2016 00:15:46 GMT
Young-Seok Kim has posted comments on this change.

Change subject: Index-only plan
......................................................................


Patch Set 11:

(21 comments)

The following three issues are discovered.
1. R-tree index on rectangle type is not correctly dealt with for the corresponding index-only
plan case
   This case should be added as a test case.
2. limit-push-down for primary index scan doesn't seem to work.
3. limit-push-down for primary-index search doesn't seem to work.

Also, please address other comments.

https://asterix-gerrit.ics.uci.edu/#/c/744/11/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractReplicateOperator.java
File algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractReplicateOperator.java:

Line 36:     private boolean[] outputMaterializationFlags = new boolean[outputArity];
I wonder why this flag is required.


https://asterix-gerrit.ics.uci.edu/#/c/744/11/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantGroupByDecorVarsRule.java
File algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantGroupByDecorVarsRule.java:

Line 45: public class RemoveRedundantGroupByDecorVarsRule implements IAlgebraicRewriteRule
{
Is this class introduced for just for optimization, not for the correctness issue?


https://asterix-gerrit.ics.uci.edu/#/c/744/11/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/PrimaryIndexSearchOperationDatasetLevelCallback.java
File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/PrimaryIndexSearchOperationDatasetLevelCallback.java:

Line 35: public class PrimaryIndexSearchOperationDatasetLevelCallback extends AbstractOperationCallback
implements ISearchOperationCallback {
This class should be removed.


https://asterix-gerrit.ics.uci.edu/#/c/744/11/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/SecondaryIndexInstantSearchOperationTryLockOnlyOnPKCallback.java
File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/opcallbacks/SecondaryIndexInstantSearchOperationTryLockOnlyOnPKCallback.java:

Line 33: public class SecondaryIndexInstantSearchOperationTryLockOnlyOnPKCallback extends
AbstractOperationCallback
Can we give a simpler class name, e.g., SecondaryIndexInstantSearchOperationCallback?


https://asterix-gerrit.ics.uci.edu/#/c/744/11/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 644:                                 .getIxJoinOuterAdditionalDataSourceVariables(i);
What's the difference between subTreeDataSourceVars and subTreePKs? why is it OK for subTreeDataSourceVars
to replace some of subTreePks below?


Line 792:             expr = (AbstractLogicalExpression) childFuncExpr.getArguments().get(0).getValue();
better be dealing with an else case explicitly, i.e., neither Assign nor unnest case.


https://asterix-gerrit.ics.uci.edu/#/c/744/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodAnalysisContext.java
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodAnalysisContext.java:

Line 65:     //    whether a verification (especially for R-Tree case) is required after the
secondary index search?
what's the difference between 3. requireVerificationAfterSIdxSearch and 4. doesSIdxSearchGenerateNoFalsePositiveResults?
It seems the same? It would be good to explain the difference with example cases.


Line 67:     //    Does the given index can cover all search predicates?
It seems that the line 67 and 69 should be switched??


https://asterix-gerrit.ics.uci.edu/#/c/744/11/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodJobGenParams.java
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodJobGenParams.java:

Line 44:     protected boolean isPrimaryIndex;
Please say that "isPrimaryIndex is a derived parameter from the index name, so this parameter
is not included in the 8 (NUM_PARAMS) parameters written and read."


https://asterix-gerrit.ics.uci.edu/#/c/744/11/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 260:         // If it is not granted, then we need to do a secondary index lookup, sort
PKs,
Do we sort in this case? ( I thought we don't do sort for this trylockfailure case?)


Line 1301:                 ((AbstractFunctionCallExpression) createOriginalSpatialObjectExpr).getArguments().addAll(expressions);
Here, the createOriginalSpatialObjectExpr should be added to restoredSecondaryKeyFieldExprs
and similarly assignRestoredSecondaryKeyFieldOp and restoredSecondaryKeyFieldVars should be
set correctly. Index-only plan test cases including R-tree index on rectangle field should
be added.


https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractLogicalOperator.java
File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/AbstractLogicalOperator.java:

Line 61:     // The code used for canDecreaseCardinality() - whether this operator can decrease
the input cardinality
Please explain the meaning of each enum value, especially for CANBEBOTH and NOTAPPLICABLE


Line 70:     public enum canPreserveOrderCode {
Please explain the meaning of each enum value, especially for CANBEBOTH and NOTAPPLICABLE


https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/LeftOuterJoinOperator.java
File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/LeftOuterJoinOperator.java:

Line 83:         return canPreserveOrderCode.CANBEBOTH;
Again, what's the meaning of CANBEBOTH, here?


https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/TokenizeOperator.java
File hyracks-fullstack/algebricks/algebricks-core/src/main/java/org/apache/hyracks/algebricks/core/algebra/operators/logical/TokenizeOperator.java:

Line 188:         return canDecreaseCardinalityCode.FALSE;
Empty string may not generate any token? If yes, should this be true?


https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantGroupByDecorVarsRule.java
File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantGroupByDecorVarsRule.java:

Line 45: public class RemoveRedundantGroupByDecorVarsRule implements IAlgebraicRewriteRule
{
Why this class is duplicated in hyracks and algebricks ?


https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantVariablesRule.java
File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveRedundantVariablesRule.java:

Line 188:                     if (equivalentVars != null && !equivalentVars.isEmpty())
{
equivalentVars are not null for sure since it's created in line 186.


https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeWithAntiMatterTuplesSearchCursor.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-rtree/src/main/java/org/apache/hyracks/storage/am/lsm/rtree/impls/LSMRTreeWithAntiMatterTuplesSearchCursor.java:

Line 276:                 proceedFailCount += 1;
remove this value or enable only if the code runs in debugging mode


Line 282:                 proceedSuccessCount += 1;
remove this value or enable only if the code runs in debugging mode


Line 327:         if (useProceedResult) {
remove this value or enable only if the code runs in debugging mode


https://asterix-gerrit.ics.uci.edu/#/c/744/11/hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/util/BinaryHashSet.java
File hyracks/hyracks-data/hyracks-data-std/src/main/java/org/apache/hyracks/data/std/util/BinaryHashSet.java:

Line 40: public class BinaryHashSet {
Why is this class not shown in the checked out codebase??


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifa02c13d4fddd880e1ee9e85eef6577301fb4560
Gerrit-PatchSet: 11
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Taewoo Kim <wangsaeu@yahoo.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Taewoo Kim <wangsaeu@yahoo.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: Young-Seok Kim <kisskys@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message