>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

Reply via email to