>From Ali Alsuliman <ali.al.solai...@gmail.com>: Attention is currently required from: Shahrzad Shirazi. Ali Alsuliman 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 20: (24 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/c782eaf2_901337ca PS20, Line 1258: LinkedHashMap<LogicalVariable, LogicalVariable> origVarToSIdxUnnestMapOpVarMap = new LinkedHashMap<>(); Don't we need this? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/b20278a6_767ae5de 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. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/dbad07e2_b69ca389 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? Keep in mind that array indexes also use distinct - sort similar to index-only plan, so for common paths between array indexes and normal indexes make sure you distinguish between the two cases properly. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/42fa7682_72f1909f PS20, Line 1341: origVarToOutputVarMap.put(pkVarsFromSIdxUnnestMapOp.get(0), newMissingPlaceHolderForLOJ); Can you check why this is needed? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/e746b083_bbd74dae PS20, Line 1544: ILogicalOperator currentOpAfterTopOp = null; What is this? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/ea6e6239_95c0703c PS20, Line 1570: ExecutionMode.LOCAL ExecutionMode.PARTITIONED? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/b25cfcb6_4e79db4d PS20, Line 1571: context.computeAndSetTypeEnvironmentForOperator(order); : VariableUtilities.substituteVariables(order, origVarToOutputVarMap, context); Do we need to swap these two lines? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/10bb7653_6c2e75e4 PS20, Line 1586: VariableUtilities.substituteVariables(distinct, origVarToOutputVarMap, context); Do you know what this is doing? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/a8a21d14_1d48c955 PS20, Line 1591: constAssignOp.getInputs().clear(); Can you double check the logic of this code with the new index-only plan? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/c95d356b_3b4525d7 PS20, Line 1598: ILogicalOperator currentOpAfterTopOp = null; What is this? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/097679f6_b9bba822 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: Case A) non-index-only plan ... Case B) index-only plan: 1. single collection query: 2. join query: 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/5a600fee_98763562 PS20, Line 317: distint missing sort on PKs 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/55f5bfda_aaf47d70 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(). Why was it removed? rebasing issues? (same below) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/98dbbdca_b0ddadc2 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. In any case, component filters have not been exercised much, I guess. Just take note of this for now and debug it later. 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/1d25f97a_472baff7 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. 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/715f2766_eaad47a0 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.0E11, doc-size: -1.0, op-cost: 0.0, total-cost: 5.00009E11]) 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/89646ad8_cce87c4b PS20, Line 32: -- HASH_PARTITION_EXCHANGE [$$42] It feels like this should not have been added. Can you file a ticket about this? File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/functions/BuiltinFunctions.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/29602e7d_b9b0afda PS20, Line 2973: WIN_PARTITION_LENGTH_IMPL Looks like a typo? 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/05a490c8_39c976c8 PS20, Line 33: WinMarkHalloweenRunningAggregateDescriptor Rename it to WinMarkValidTuples... Add a documentation up describing the purpose of the function with an example plan snippet. 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/b8a7f7f2_50d8339d PS20, Line 74: IPointable[] firstSKForTheRecord = null; The position is out of place. Move it up, make it private final. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/4a57a60e_53d2cc73 PS20, Line 86: firstSKForTheRecord = new IPointable[argEvals.length]; We should create this only once and re-use/reset. Otherwise, we will be creating many objects for each PK partition https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/5a76ffa6_8cc5c4c4 PS20, Line 89: argEval.evaluate(tuple, argValue); : firstSKForTheRecord[i] = argValue; This is overriding the previous values. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/a030dc20_3d2d9508 PS20, Line 95: notDuplicateRecord Rename it to "sameTuple" https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17729/comment/f2a414b0_30a90a3f PS20, Line 109: contentEquals We need to use our comparators. -- 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: 20 Gerrit-Owner: Shahrzad Shirazi <shaji...@ucr.edu> Gerrit-Reviewer: Ali Alsuliman <ali.al.solai...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Attention: Shahrzad Shirazi <shaji...@ucr.edu> Gerrit-Comment-Date: Thu, 07 Aug 2025 22:47:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment