Dmitry Lychagin has posted comments on this change. Change subject: [ASTERIXDB-1972][COMP][RT][TX] index-only plan ......................................................................
Patch Set 56: (14 comments) Can you also rebase? We got build failure in CB tests, rebasing should fix it. https://asterix-gerrit.ics.uci.edu/#/c/1866/56/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: PS56, Line 380: int keyType let's create an enum for requested keys. 0,1,2 is not very informative Line 397: if (outputPrimaryKeysOnlyFromSIdxSearch) { it's a bit strange that we need do cannot tell this information from the operator itself, but instead rely on the function parameter. I think currently outputPrimaryKeysOnlyFromSIdxSearch is set to true only for inverted indexes. can we just set it here by looking at the index type instead? Line 414: // Fetches conditional splitter - the last position let's add a sanity check here that unnestMapOp has getGenerateCallBackProceedResultVar set to true. Line 416: stop = sourceVars.size(); should we just set stop = start + 1 here? sourceVars might have more variables if propagateIndexFilter = true Line 427: public static List<LogicalVariable> getPrimaryKeyVarsFromSecondaryUnnestMap(Dataset dataset, this method is no longer used. let's remove it. the functionality is provided by the new getKeyVarsFromSecondaryUnnestMap() method Line 514: if (realTypeConvertedToIntegerType) { 1) it seems like this floor/ceil conversion is needed for index on closed-field (as the comment below indicates). For other indexes relaxing condition from < to <= would still work fine as before. So can we limit it to this case by changing this condition to "realTypeConvertedToIntegerType && !index.isEnforced() && !index.isOverridingKeyFieldTypes()" 2) The precision loss is also possible when converting a large BIGINT to FLOAT/DOUBLE. I wonder whether we need to deal with this in the new code? Line 578: break; throw IllegalStateException here. NEQ is not expected here. Line 583: if (mathFunctionTypeForNumericTypeCasting == TypeCastingMathFunctionType.NONE) { this should probably be "else if" from the previous "if" because mathFunctionTypeForNumericTypeCasting is assigned there to CEIL/FLOOR. Also the EQ case is weird because it does not assign mathFunctionTypeForNumericTypeCasting and therefore causes this if statement to run which will reassign replacedConstantValue. I think this problem would go away if we make this "else if" and we'll no longer need variable for mathFunctionTypeForNumericTypeCasting because it won't be used outside of the switch. Line 602: // We are optimizing a join query. Determine which variable feeds the secondary index. this used to be in the 'else' branch. if (probeSubTree == null) { // We are optimizing a selection query. } else { // We are optimizing a join query. } Let's bring 'else' back Line 949: List<LogicalVariable> skVarsFromSIdxUnnestMap = AccessMethodUtils.getKeyVarsFromSecondaryUnnestMap(dataset, skVarsFromSIdxUnnestMap are used only for the index-only plan. let's move it below into the index-only plan section. Line 955: boolean skFieldUsedAfterTopOp = indexOnlyPlanInfo.getSecond(); skFieldUsedAfterTopOp and skFieldUsedAfterTopOp are not used for non index-only plan. let's move them below into index-only plan section Line 965: // From now on, we deal with the index-only plan. We now have createRestOfNonIndexOnlySearchPlan() method for dealing with non index-only plans. Let's refactor the rest of this method into createRestOfIndexOnlySearchPlan() method to be consistent. Line 1833: if (isInvertedIndex(chosenIndex)) { Can we move this check to the top of this method? We know whether the index is the inverted index, so why bother with all above checks at all if the index plan cannot be used for it? https://asterix-gerrit.ics.uci.edu/#/c/1866/56/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java: Line 183: // Secondary index search in an index-only plan case The assumption here is that if we see secondary index access then it's from the index only plan. How can we be sure about that? Do we need to check that it is the case or not? Also, this else branch looks exactly the like the if branch, only it sets INDEXONLY_PLAN_SECONDARY_INDEX_LOOKUP instead of PRIMARY_INDEX_LOOKUP. Does it make sense to combine the two: DataSourceType dsType = jobGenParams.isPrimaryIndex() ? DataSourceType.PRIMARY_INDEX_LOOKUP : DataSourceType.INDEXONLY_PLAN_SECONDARY_INDEX_LOOKUP; then just use 'dsType' -- 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: 56 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Taewoo Kim <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Ildar Absalyamov <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
