asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yingyi Bu (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: [ASTERIXDB-1972][COMP][RT][TX] index-only plan
Date Tue, 11 Jul 2017 01:06:30 GMT
Yingyi Bu has posted comments on this change.

Change subject: [ASTERIXDB-1972][COMP][RT][TX] index-only plan
......................................................................


Patch Set 13:

(31 comments)

https://asterix-gerrit.ics.uci.edu/#/c/1866/13/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 595:                         } catch (HyracksDataException e) {
> CRITICAL SonarQube violation:
fix this?


Line 608:                         } catch (HyracksDataException e) {
> CRITICAL SonarQube violation:
Fix this?


PS13, Line 750:  // Are we transforming a join plan?
considering keeping the old method and create a new one for index only?


PS13, Line 752: topOpRef.getValue().getOperatorTag() != LogicalOperatorTag.SELECT
is there any chance to make the code agnostic to SELECT?


PS13, Line 939: // If there are ASSIGN operators before the given SELECT or JOIN operator,
we need to propagate
              :             // these variables to the UNIONALL operator, too.
update the comment to be based on variable usage/liveness.


PS13, Line 962: if (afterTopOpRefs != null) {
              :                 for (Mutable<ILogicalOperator> afterTopOpRef : afterTopOpRefs)
{
              :                     tmpVars.clear();
              :                     VariableUtilities.getUsedVariables((ILogicalOperator)
afterTopOpRef.getValue(), tmpVars);
              :                     for (LogicalVariable tmpVar : tmpVars) {
              :                         if (!usedVarsAfterTopOp.contains(tmpVar)) {
              :                             usedVarsAfterTopOp.add(tmpVar);
              :                         }
              :                     }
              :                 }
              :             }
OperatorPropertiesUtils.getFreeVaraibles(....)


PS13, Line 977: 
              :             // Is the used variables after SELECT operator from the primary
index?
              :      
UnionAll need to generate new variable, so that the query plan can stay as SSA form.  

You have a top-down pass to keep variables that needs to be retained and have a bottom-up
pass to replace variables according to the mapping produced by the down stream.


PS13, Line 1077: unionVarMap.add(new Triple<>(v, v, v));
make v/v/v as SSA?

v1/v2/v3


PS13, Line 1376: else {
               :             // Non-index-only plan optimization - still index-utilization
plan.
               :             return primaryIndexUnnestOp;
               :         }
Pull that into a separate if block and then the current if branch doesn't need nesting.


PS13, Line 1644: ILogicalOperator tmpOp = indexSubTree.getRoot();
               :             while (tmpOp.getOperatorTag()
OperatorPropertiesUtil.getFreeVariables(...)


PS13, Line 2396:   return unionAllOp;
               :         } else {
               :             return unionOp;
               :         }
flip condition and reduce if branch.


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/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 95:     private static List<Pair<FunctionIdentifier, Boolean>> FUNC_IDENTIFIERS
= Collections.unmodifiableList(
> MAJOR SonarQube violation:
final?


PS13, Line 155: secondaryKeyFieldUsedAfterSelectOp 
this variable is not needed for B-tree?


PS13, Line 218: || dataSourceRefOp.getOperatorTag() == LogicalOperatorTag.LEFT_OUTER_UNNEST_MAP
Is the or branch covered by any tests?


PS13, Line 232:  // The whole plan is now changed into an index-only plan
you only use primary index for this branch, why it is related to index only?


PS13, Line 240: dataSourceRefOp.getOperatorTag() == LogicalOperatorTag.LEFT_OUTER_UNNEST_MAP
same as above


PS13, Line 305: boolean secondaryKeyFieldUsedAfterJoinOp = false;
Is this variable necessary?


PS13, Line 349: IsNull
IsNull -> isMissing?


PS13, Line 391: primaryIndexUnnestOp
rename the variable?

Replace the if-else with an one line conditional expression.


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

PS13, Line 384: intersectAllSecondaryIndexes
factor out single index handling as another method and call the new method in the else block.


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/RTreeAccessMethod.java
File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/RTreeAccessMethod.java:

PS13, Line 240: generateInstantTrylockResultFromIndexSearch = true;
change that to an one line assignment.


PS13, Line 425: isIndexOnlyPlan && dataset.getDatasetType() == DatasetType.INTERNA
Share the code with BTreeAccessMethod as much as possible?


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/AbstractOperationCallbackFactory.java
File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/AbstractOperationCallbackFactory.java:

PS13, Line 34: protected boolean isIndexOnlyPlanEnabled;
Move to this flag to the specific call back need that?


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java
File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java:

PS13, Line 558:  public
copy the documentation here if it is public.


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/hierachy/DoubleToInt16TypeConvertComputer.java
File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/types/hierachy/DoubleToInt16TypeConvertComputer.java:

PS13, Line 62:  switch (mathFunction) {
             :             case CEIL:
             :                 sourceValue = Math.ceil(((ADouble) sourceObject).getDoubleValue());
             :                 break;
             :             case FLOOR:
             :                 sourceValue = Math.floor(((ADouble) sourceObject).getDoubleValue());
             :                 break;
             :             default:
             :                 sourceValue = ((ADouble) sourceObject).getDoubleValue();
             :                 break;
             :         }
mathFunction.covert(sourceObject, ...);


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/utils/Quadruple.java
File hyracks-fullstack/algebricks/algebricks-common/src/main/java/org/apache/hyracks/algebricks/common/utils/Quadruple.java:

Line 18:     public T1 first;
> MAJOR SonarQube violation:
final?


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/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:

PS13, Line 252:  AbstractLogicalOperator op = (AbstractLogicalOperator) r.getValue();
              :         for (Mutable<ILogicalOperator> i : op.getInputs()) {
              :             typeOpRec(i, context);
              :         }
              :         if (op.hasNestedPlans()) {
              :             for (ILogicalPlan p : ((AbstractOperatorWithNestedPlans) op).getNestedPlans())
{
              :                 typePlan(p, context);
              :             }
              :         }
              :         context.computeAndSetTypeEnvironmentForOperator(op);
return typeOpRec(r.getValue(), context);


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/IntroduceProjectsRule.java
File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/IntroduceProjectsRule.java:

PS13, Line 186: boolean projectBeforeUnionFound = false;
Make the rule general and agnostic to specific operators.

One fix could be to not eliminate a project operator if it perturbs the variable ordering
even if it doesn't change liveness of variables.


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveUnusedAssignAndAggregateRule.java
File hyracks-fullstack/algebricks/algebricks-rewriter/src/main/java/org/apache/hyracks/algebricks/rewriter/rules/RemoveUnusedAssignAndAggregateRule.java:

PS13, Line 156: .getOperatorTag() == LogicalOperatorTag.AGGREGATE
              :                     || (op.getOperatorTag() == LogicalOperatorTag.UNIONALL
&& keepUnionOp
Fix it here instead of adding a special case handling method.


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeRangeSearchCursor.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-btree/src/main/java/org/apache/hyracks/storage/am/lsm/btree/impls/LSMBTreeRangeSearchCursor.java:

PS13, Line 54: private byte[] returnValuesArrayForProccedResult = new byte[10];
split into two byte arrays.

Passing those values through CursorFactory instead of opContext.


https://asterix-gerrit.ics.uci.edu/#/c/1866/13/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndexOperationContext.java
File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractLSMIndexOperationContext.java:

PS13, Line 157: @Override
              :     public void setUseOperationCallbackProceedReturnResult(boolean useOperationCallbackProceedReturnResult)
{
              :         // Not applicable for this class.
              :     }
make the information passed through constructors instead of mutable methods.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd5c9ab1cf2e4bedb7d8db582441919875e74d51
Gerrit-PatchSet: 13
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Taewoo Kim <wangsaeu@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Taewoo Kim <wangsaeu@gmail.com>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message