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 Thu, 07 Apr 2016 17:41:47 GMT
Young-Seok Kim has posted comments on this change.

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


Patch Set 7:

(19 comments)

Please address comments

https://asterix-gerrit.ics.uci.edu/#/c/744/7/asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java
File asterix-algebra/src/main/java/org/apache/asterix/algebra/operators/physical/InvertedIndexPOperator.java:

Line 290:                 // We deduct 1 because the last field will be the result of searchCallback.proceed()
1) If I remember correctly, the index-only plan for the inverted index is not possible due
to the instantlock, right?
2) Even though the index-only plan is not possible for now, it's good to have more comments
here to explain the layout of the fields in the inverted index. For example, an entry of inverted
index consists of [SK, PK] and when the index only plan is enabled, where the additional field
for the trylock result is appended. With this explanation, the following -1 and primaryKeyFieldsInSecondaryIndex
array variable can be more clearly understandable.

The same comments for other indexes.


Line 301:                                 primaryKeyFieldsInSecondaryIndex, txnSubsystemProvider,
ResourceType.LSM_BTREE,
should be LSM_INVERTED_INDEX?


Line 320:             valuesForIndexOnlyPlan = castBuffer.getByteArray();
This byte array is only required for index-only plan case. So, code creating the byte array
should be executed only for index-only plan by moving the code into above else clause. If
it's not index-only plan the byte array could be null.

The same comments for other indexes.


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

Line 669:                 if (subTree.dataSourceType != DataSourceType.COLLECTION_SCAN) {
What's the reason for ignoring the COLLECTION_SCAN?


Line 1036:                                 UnnestMapOperator newUnnestMapOp = replaceExpressionOfUnnestMapOperator(jobGenParams,
Why can't the existing unnestMapOp be reused? Why is a new unnestMapOp required?


Line 1057:                                 UnnestMapOperator newUnnestMapOp = replaceExpressionOfUnnestMapOperator(jobGenParams,
Why can't the existing unnestMapOp be reused? Why is a new unnestMapOp required?


Line 1086:         return false;
let's remove this unreachable code.


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

Line 745:             // For the composite index, a secondary index search generates a superset
of the results.
Why does the composite index generate a superset of the results?


Line 1625:                 //                context.computeAndSetTypeEnvironmentForOperator(lastAssignBeforeTopOp);
Is the above code not sufficient? Is the following for clause needed?


Line 1760:             //            unionAllOp.getInputs().add(new MutableObject<ILogicalOperator>(currentTopOpInRightPath));
better be removing the above two lines if they are not used. Also, if the following three
lines are needed for the debugging purpose, introduce DEBUG flag as private static final boolean
and set it to false. Then put the 3 lines into if clause as follows:
if (DEBUG) {
    //3 lines here. 
}


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

Line 656: 
Why do we do this? why can't we remove false positives in this composite key case?


Line 657:         // If the select condition contains mixed open/closed intervals on multiple
keys, then we make all intervals closed to obtain a superset of answers and leave the original
selection in place.
typo:primaryIndexPostProccessingIsNeeded -> primaryIndexPostProcessingIsNeeded


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

Line 176:             if (op.getOperatorTag() != LogicalOperatorTag.SINK) {
Can we make a private function isRootOp(op) to replace these two if clauses? This will hide
the details such as which operators can be root operators.


Line 293:     protected boolean checkAndApplyTheRule(Mutable<ILogicalOperator> opRef,
int nthChild) throws AlgebricksException {
nthChild variable is not used. Let's remove it.


Line 311:                     if (subTree.selectRefs == null || (subTree.selectRefs != null
&& subTree.selectRefs.size() == 0)) {
In which AQL query, does the above if condition become false?
If there is such an AQL query, it's good to make the query as a test case.
If such a query is covered by a test case already, please explain the case here and mention
the tc in comments here.


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

Line 189:                 if (subTreeOp.getInputs().size() < 1 || subTreeOp.getInputs()
== null) {
should be as follows to avoid null pointer exception:
if (subTreeOp.getInputs() == null || subTreeOp.getInputs().size() < 1 )


Line 278:     private boolean initFromIndexOnlyOrReducingTheNumberOfTheSelectVerficationPlan(
This method should be split into two methods, one for indexOnly and one for reducingSelectVerfication.
It seems no reason to put these two logics in a same method.


Line 550:         // Control-flow should not reach here.
remove unreachable code.


https://asterix-gerrit.ics.uci.edu/#/c/744/7/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java
File asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java:

Line 740:             AqlMetadataImplConfig aqlMetadataImplConfig = (AqlMetadataImplConfig)
implConfig;
impleConfig is created and set when IntroduceInstantLockSearchCallbackRule is fired. This
rule is useless since we decided to support read-committed isolation level. From the rulecollection,
the rule should be commented out. So, regardless of whether a dataset is seen more than once
or not, we will always use instantLock. Therefore, the code from line760~783 should be changed
to use only instant lock. No dataset level lock!


-- 
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: 7
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Taewoo Kim <wangsaeu@yahoo.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
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