>From Shahrzad Shirazi <[email protected]>: Shahrzad Shirazi has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729 )
Change subject: [NO ISSUE][COMP] Simplify Index-only plan ...................................................................... Patch Set 43: (23 comments) File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/AccessMethodUtils.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/9da0eed4_60efdfc3 PS20, Line 1258: LinkedHashMap<LogicalVariable, LogicalVariable> origVarToSIdxUnnestMapOpVarMap = new LinkedHashMap<>(); > Don't we need this? Every needed in this is handled by origVarToOutputVarMap in the new code and it will keep the mapping between the old version of the pk and sks to the new ones. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/e97fb0b2_ff52b28c PS20, Line 856: ScalarFunctionCallExpression lojFuncExprs = analysisCtx.getLOJIsMissingNullFuncInSpecialGroupBy(); : List<LogicalVariable> lojMissingNullVariables = new ArrayList<>(); : lojFuncExprs.getUsedVariables(lojMissingNullVariables); > These 3 lines do not seem to be needed anymore, right? Remove if not needed. Yes,removed https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/930a3cf7_0c69d11b PS20, Line 931: indexSubTree.getRootRef().getValue(); > Can you check what this is doing now for the index-only plan and whether we > need a special check? Ke […] Sure! In general, if the primary index search fully covers the SELECT condition, the condition is set to null, and the index search becomes the final operation (e.g., in an IndexNestedLoopJoin). If parts of the condition remain, we reconstruct a new condition from the uncovered parts. The removed if check was specific to UNION ALL. It wasn't checking for index-only plans directly, but handled a case where "newMissingNullPlaceHolderVar" was missing in the union's variable map in index only case which will not be needed in new index-only plans. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/9a36a2ff_aedebff8 PS20, Line 1544: ILogicalOperator currentOpAfterTopOp = null; > What is this? It was not needed, the constAssignVars are handled before in the createFinalIndexOnlySearchPlan function. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/6bd90aee_3eb01519 PS20, Line 1570: ExecutionMode.LOCAL > ExecutionMode. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/5c788f46_5ab1d3fe PS20, Line 1571: context.computeAndSetTypeEnvironmentForOperator(order); : VariableUtilities.substituteVariables(order, origVarToOutputVarMap, context); > Do we need to swap these two lines? Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/6ede6aae_9b0317a8 PS20, Line 1586: VariableUtilities.substituteVariables(distinct, origVarToOutputVarMap, context); > Do you know what this is doing? It is not needed( is not doing anything) as the origVarToOutputVarMap has the mapping for the old to the new pks and sks we have already substituted the variables for the inputs of the distinct op so there is no need to do this. But still checking. Will remove in the final patch set after the final reveiw. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/b28fd4b1_9d4bd5fa PS20, Line 1591: constAssignOp.getInputs().clear(); > Can you double check the logic of this code with the new index-only plan? similar to the other one this is not needed. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/54380d8d_26e5587b PS20, Line 1598: ILogicalOperator currentOpAfterTopOp = null; > What is this? It was not needed, the constAssignVars are handled before in the createFinalIndexOnlySearchPlan function. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/71becc7b_edcae07b PS20, Line 1652: Creates operators that do a primary index lookup in the plan or an index-only query plan. > We should keep and update the comments saying: […] Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/BTreeAccessMethod.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/bd384356_93210c57 PS20, Line 317: distint > missing sort on PKs Done File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IntroduceLSMComponentFilterRule.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/703e0515_b8901629 PS20, Line 99: IAType filterSourceType = filterSourceIndicator == null || filterSourceIndicator == 0 : ? mp.findType(dataset.getItemTypeDatabaseName(), dataset.getItemTypeDataverseName(), : dataset.getItemTypeName()) : : mp.findType(dataset.getMetaItemTypeDatabaseName(), dataset.getMetaItemTypeDataverseName(), : dataset.getMetaItemTypeName()); > We need the mp.findTypeForDatasetWithoutType(). […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/97478e1b_4b23099f PS20, Line 298: case ORDER: : case DISTINCT: : ILogicalOperator child = intersectOrSort.getValue().getInputs().get(0).getValue(); > This seems already broken. Also, heterogeneous indexes might have issues with > this. […] Ack File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/RTreeAccessMethod.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/cb6e584b_67309af3 PS20, Line 210: If this is an index-only plan, the topmost operator returned is UNIONALL operator. > Update this comment to say DISTINCT for single collection query & SELECT for > join query. Ack File asterixdb/asterix-app/src/test/resources/optimizerts/results/btree-index-join/leftouterjoin-probe-pidx-with-join-btree-sidx_01_ps.plan: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/1081befc_c1e68a56 PS20, Line 1: [cardinality: 5.0E11, doc-size: -1.0, op-cost: 0.0, total-cost: 5.00009E11] > Don't include the stats in the optimizer tests (optimizerts) (e.g. > [cardinality: 5. […] Done File asterixdb/asterix-app/src/test/resources/optimizerts/results/btree-index-join/leftouterjoin-probe-pidx-with-join-btree-sidx_03-index-only.plan: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/1ba0d4ad_3359cb37 PS20, Line 32: -- HASH_PARTITION_EXCHANGE [$$42] > It feels like this should not have been added. […] Done File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/8009261c_06e38758 PS20, Line 2973: WIN_PARTITION_LENGTH_IMPL > Looks like a typo? Ack File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/WinMarkHalloweenRunningAggregateDescriptor.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/22dfbeae_c3cf2f67 PS20, Line 33: WinMarkHalloweenRunningAggregateDescriptor > Rename it to WinMarkValidTuples... […] Sorry forgot about the second section will add it in the last version. File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/runningaggregates/std/WinMarkHalloweenRunningAggregateEvaluator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/29bc5edb_d135f17e PS20, Line 74: IPointable[] firstSKForTheRecord = null; > The position is out of place. Move it up, make it private final. Ack https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/0000fbee_d8d2b529 PS20, Line 86: firstSKForTheRecord = new IPointable[argEvals.length]; > We should create this only once and re-use/reset. […] Ack https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/c2474336_663e89c8 PS20, Line 89: argEval.evaluate(tuple, argValue); : firstSKForTheRecord[i] = argValue; > This is overriding the previous values. Ack https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/85b26bda_d77e5ac0 PS20, Line 95: notDuplicateRecord > Rename it to "sameTuple" Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/49e88c48_99e01fc3 PS20, Line 109: contentEquals > We need to use our comparators. Ack -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I516dc90b8da3a7086dc80b67946a5d4f6dde0972 Gerrit-Change-Number: 17729 Gerrit-PatchSet: 43 Gerrit-Owner: Shahrzad Shirazi <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Comment-Date: Mon, 18 Aug 2025 17:49:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Ali Alsuliman <[email protected]> Gerrit-MessageType: comment
