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 <wangs...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Taewoo Kim <wangs...@gmail.com> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-HasComments: Yes
