Skip to content

Projection pushdown below Exchange + QTF adaptation - #22082

Merged
sandeshkr419 merged 15 commits into
opensearch-project:mainfrom
expani:project_pushdown
Jun 13, 2026
Merged

Projection pushdown below Exchange + QTF adaptation#22082
sandeshkr419 merged 15 commits into
opensearch-project:mainfrom
expani:project_pushdown

Conversation

@expani

@expani expani commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

What

Push Project below the Exchange so DataFusion reads only referenced columns from parquet on each shard, instead of scanning all ~105 columns and projecting at the coordinator.

Why

PPL queries like source=idx | where x | fields body were reading every column off parquet and shipping them across the gather. Wasteful I/O and network.

Changes

  1. PlannerImpl.pushdownRules — remove SORT_PROJECT_TRANSPOSE. Keeps the SELECT Project below the Sort/Exchange so DataFusion's OptimizeProjections prunes the scan.
  2. OpenSearchExchangeReducer.computeSelfCost — scale cost by output column count (× getRowType().getFieldCount()). Volcano now prefers placing the Exchange where fewer columns cross it.
  3. OpenSearchLateMaterializationRewriter — adapt QTF, which previously assumed SORT_PROJECT_TRANSPOSE lifted the Project above the Sort:
    • Targeted detection gate: decline only on a derived sort key or a window function below the anchor (was: any expression).
    • Relocate the SELECT projection above the late-mat wrapper, refs rebased scan→wrapper space (aliases + expressions preserved).
    • Collapse the resulting adjacent Projects into one.

Verified

  • DF physical plan reads only referenced columns (QTF declines) / narrowed sort-key scan + post-fetch projection (QTF fires) - confirmed on a live 2-shard node.
  • LateMaterialization suite green, incl. new tests for alias preservation, mixed-direction multi-key sort, and expressions where an operand is also the sort key.
  • No regression in Aggregate / Window / Join / Union / Subquery plan-shape suites.

Notes for reviewers

The QTF change is a consequence of removing SORT_PROJECT_TRANSPOSE, not a feature — it preserves QTF behavior (same fire set, same scan narrowing, same deferred-fetch set), not improves it.

@expani
expani requested a review from a team as a code owner June 9, 2026 12:09
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 0bff4dc)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

In buildProjectAboveWrapper, when an expression slot in the below-Project references a scan column that was not fetched (scanToWrapper[ref.getIndex()] returns -1), the code continues without adding the rebased expression to the output. This silently drops the expression slot, corrupting the output schema: the wrapper output will have fewer columns than the original below-Project, breaking any above-chain operator that references the dropped slot by its original index. Trigger: a below-Project with an expression (e.g., UPPER(URL)) whose operand is a sort-only column not included in aboveAnchorPhysicalFields.

RexNode expr = innerProject.getProjects().get(slot);
RexNode rebased;
if (expr instanceof RexInputRef ref) {
    int w = scanToWrapper[ref.getIndex()];
    if (w < 0) continue; // sort-only passthrough column — not fetched, drop
    rebased = new RexInputRef(w, wrapperFields.get(w).getType());
} else {
    rebased = expr.accept(toWrapper); // expression: all physical deps are fetched
Possible Issue

collationReferencesDerivedSlot declines QTF when any anchor collation index lands on an expression slot (-1) in the below-Project. However, the detection phase computes belowAnchorPhysicalFields by walking the anchor's collation and resolving each index through belowProjOutToScan to a scan column name. If belowProjOutToScan[fc.getFieldIndex()] is -1 (expression slot), the code at line 289 attempts to use -1 as a scan column index, throwing IndexOutOfBoundsException or reading the wrong column. Trigger: anchor sorts on a derived below-Project column (e.g., ORDER BY UPPER(URL)).

private static boolean collationReferencesDerivedSlot(OpenSearchSort anchor, int[] belowProjOutToScan) {
    for (RelFieldCollation fc : anchor.getCollation().getFieldCollations()) {
        if (belowProjOutToScan[fc.getFieldIndex()] < 0) return true;
    }
    return false;
}

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 0bff4dc

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Verify trailing helpers are present

The nested loop searches for each helper by name in childFields but does not verify
that the helper was actually found. If a helper is missing from childFields, it will
be silently skipped, potentially causing downstream issues. Add a check to ensure
each expected helper is present, or log a warning if it's absent.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [579-588]

 for (String helper : OpenSearchLateMaterialization.TRAILING_HELPERS_IN_ORDER) {
+    boolean found = false;
     for (int i = 0; i < childFields.size(); i++) {
         if (helper.equals(childFields.get(i).getName())) {
             newProjects.add(new RexInputRef(i, childFields.get(i).getType()));
             newNames.add(helper);
+            found = true;
             break;
         }
     }
+    if (!found) {
+        throw new IllegalStateException("Expected trailing helper '" + helper + "' not found in child fields");
+    }
 }
Suggestion importance[1-10]: 7

__

Why: This suggestion correctly identifies a potential silent failure when expected trailing helpers (like ___row_id or ___ugsi) are missing from childFields. Adding validation ensures the rewriter fails fast with a clear error message rather than producing a corrupted plan, improving debuggability and correctness.

Medium
General
Validate index remapping handles sentinel values

The scanToWrapper mapping uses -1 to indicate a column was not fetched, but the
subsequent IndexRemapShuttle may not handle -1 indices correctly if they appear in
expression operands. Verify that IndexRemapShuttle properly rejects or handles -1
indices to prevent invalid references from propagating into the rewritten plan.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [429-433]

 int[] scanToWrapper = new int[scanFields.size()];
 for (int k = 0; k < scanFields.size(); k++) {
     Integer w = wrapperIdxByName.get(scanFields.get(k).getName());
     scanToWrapper[k] = (w == null) ? -1 : w;
 }
+// Ensure IndexRemapShuttle properly rejects -1 indices during rebasing
Suggestion importance[1-10]: 5

__

Why: The suggestion raises a valid concern about -1 sentinel values in scanToWrapper being passed to IndexRemapShuttle. However, the comment-only improvement doesn't add runtime validation. The concern is legitimate but the proposed fix is incomplete—actual validation logic would be needed for a higher score.

Low
Add bounds check for negative indices

The condition arg < exprs.size() may not prevent index-out-of-bounds errors if arg
is negative. Although unlikely in practice, argList could theoretically contain
negative indices. Add an explicit check for arg >= 0 to ensure robustness and
prevent potential runtime exceptions.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchAggLiteralArgProjectSplitRule.java [82-86]

 for (int arg : ac.getArgList()) {
-    if (arg < exprs.size() && exprs.get(arg) instanceof RexLiteral) {
+    if (arg >= 0 && arg < exprs.size() && exprs.get(arg) instanceof RexLiteral) {
         referencesLiteralArg = true;
         break;
     }
 }
Suggestion importance[1-10]: 3

__

Why: While adding arg >= 0 improves robustness, negative indices in argList are highly unlikely in practice given Calcite's internal validation. The suggestion is technically correct but addresses an edge case with minimal real-world impact.

Low

Previous suggestions

Suggestions up to commit 9438fa9
CategorySuggestion                                                                                                                                    Impact
General
Handle infinite/NaN row counts

The cost calculation multiplies rows by field count without checking if rows is
infinite or NaN. When metadata query returns unknown cardinality, this can produce
invalid cost values that break Volcano's cost comparisons, potentially causing the
optimizer to fail or select suboptimal plans.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rel/OpenSearchExchangeReducer.java [161-165]

 @Override
 public RelOptCost computeSelfCost(RelOptPlanner planner, RelMetadataQuery mq) {
     double rows = mq.getRowCount(getInput());
+    if (Double.isInfinite(rows) || Double.isNaN(rows)) {
+        rows = 1000.0; // fallback estimate
+    }
     double widthFactor = getRowType().getFieldCount();
     return planner.getCostFactory().makeCost(SETUP_COST + rows * widthFactor, SETUP_COST + rows * widthFactor, 0);
 }
Suggestion importance[1-10]: 8

__

Why: The suggestion addresses a real issue where mq.getRowCount() can return Infinity or NaN when cardinality is unknown, leading to invalid cost calculations that break Volcano's optimizer. Adding a fallback estimate ensures cost comparisons remain valid, preventing optimizer failures or suboptimal plan selection.

Medium
Possible issue
Add array bounds validation

Add bounds checking before accessing belowProjOutToScan array. The
fc.getFieldIndex() could potentially be out of bounds if the collation references a
field index beyond the below-Project's output size, causing an
ArrayIndexOutOfBoundsException.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [275-280]

 private static boolean collationReferencesDerivedSlot(OpenSearchSort anchor, int[] belowProjOutToScan) {
     for (RelFieldCollation fc : anchor.getCollation().getFieldCollations()) {
-        if (belowProjOutToScan[fc.getFieldIndex()] < 0) return true;
+        int idx = fc.getFieldIndex();
+        if (idx < 0 || idx >= belowProjOutToScan.length || belowProjOutToScan[idx] < 0) return true;
     }
     return false;
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential ArrayIndexOutOfBoundsException if fc.getFieldIndex() exceeds belowProjOutToScan.length. However, in a well-formed plan, the collation indices should align with the below-Project's output size, making this an edge case. The fix is valid but addresses a scenario that may not occur in practice.

Medium
Validate scan-to-wrapper column mapping

The scanToWrapper mapping assumes scan fields and wrapper fields align by name, but
doesn't validate that all required scan columns are present in the wrapper. If a
scan column referenced by the below-Project is missing from the wrapper output, the
mapping will be -1 and subsequent IndexRemapShuttle operations may fail silently or
produce incorrect results.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [425-430]

 int[] scanToWrapper = new int[scanFields.size()];
 for (int k = 0; k < scanFields.size(); k++) {
     Integer w = wrapperIdxByName.get(scanFields.get(k).getName());
     scanToWrapper[k] = (w == null) ? -1 : w;
 }
+// Validate that all non-expression slots in the below-Project can be resolved
+for (int slot = 0; slot < innerProject.getProjects().size(); slot++) {
+    RexNode expr = innerProject.getProjects().get(slot);
+    if (expr instanceof RexInputRef ref && scanToWrapper[ref.getIndex()] < 0) {
+        throw new IllegalStateException("Below-Project references scan column " + scanFields.get(ref.getIndex()).getName() + " not present in wrapper output");
+    }
+}
Suggestion importance[1-10]: 6

__

Why: The suggestion adds validation to ensure all passthrough refs in the below-Project resolve to wrapper columns. While this could catch misalignment bugs, the rewrite logic already ensures all physical deps are fetched into the wrapper, so this validation is redundant in the normal case. It's a defensive check that may help during development but isn't critical.

Low
Suggestions up to commit 540429c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add array bounds checking

Add bounds checking before accessing belowProjOutToScan array to prevent potential
ArrayIndexOutOfBoundsException. The collation field index could exceed the array
length if the anchor's collation references a field beyond the below-Project's
output schema.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [275-280]

 private static boolean collationReferencesDerivedSlot(OpenSearchSort anchor, int[] belowProjOutToScan) {
     for (RelFieldCollation fc : anchor.getCollation().getFieldCollations()) {
-        if (belowProjOutToScan[fc.getFieldIndex()] < 0) return true;
+        int fieldIndex = fc.getFieldIndex();
+        if (fieldIndex >= belowProjOutToScan.length || belowProjOutToScan[fieldIndex] < 0) return true;
     }
     return false;
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential ArrayIndexOutOfBoundsException if the anchor's collation references a field index beyond the belowProjOutToScan array length. Adding bounds checking prevents runtime errors and makes the code more robust.

Medium
Validate aggregate argument indices

The bounds check arg < exprs.size() prevents out-of-bounds access, but silently
skips invalid indices. If an aggregate references an out-of-range argument, this
indicates a malformed plan that should be detected rather than ignored. Consider
logging a warning or throwing an exception for invalid argument indices.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchPercentileLiteralArgRule.java [86-97]

 for (AggregateCall ac : aggregate.getAggCallList()) {
     if (!PERCENTILE_APPROX.equalsIgnoreCase(ac.getAggregation().getName())) {
         continue;
     }
     for (int arg : ac.getArgList()) {
-        if (arg < exprs.size() && exprs.get(arg) instanceof RexLiteral) {
+        if (arg >= exprs.size()) {
+            throw new IllegalStateException("Aggregate argument index " + arg + " exceeds Project output size " + exprs.size());
+        }
+        if (exprs.get(arg) instanceof RexLiteral) {
             referencesLiteralArg = true;
             break;
         }
     }
 }
Suggestion importance[1-10]: 3

__

Why: While the suggestion raises a valid concern about silently skipping invalid indices, the current bounds check arg < exprs.size() is a defensive guard that prevents crashes. Throwing an exception might be too aggressive for a rule that should gracefully decline when conditions aren't met. The suggestion has merit but the severity is overstated.

Low
General
Handle duplicate field names

The mapping logic assumes scan field names are unique, but duplicate names would
cause the last occurrence to overwrite earlier entries in wrapperIdxByName. This
could lead to incorrect index remapping if the schema contains duplicate column
names. Verify uniqueness or handle duplicates explicitly.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [429-433]

 int[] scanToWrapper = new int[scanFields.size()];
 for (int k = 0; k < scanFields.size(); k++) {
-    Integer w = wrapperIdxByName.get(scanFields.get(k).getName());
-    scanToWrapper[k] = (w == null) ? -1 : w;
+    String fieldName = scanFields.get(k).getName();
+    Integer w = wrapperIdxByName.get(fieldName);
+    if (w == null) {
+        scanToWrapper[k] = -1;
+    } else {
+        scanToWrapper[k] = w;
+        wrapperIdxByName.remove(fieldName);
+    }
 }
Suggestion importance[1-10]: 2

__

Why: The suggestion misunderstands the code's intent. The wrapperIdxByName map is built from wrapperFields (not scan fields), and removing entries after use would break subsequent lookups. The concern about duplicate names is valid in theory, but the suggested fix is incorrect and would cause bugs.

Low
Suggestions up to commit c573cb2
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add bounds check for array access

The method accesses belowProjOutToScan[fc.getFieldIndex()] without bounds checking.
If the collation references a field index beyond the array length, this will throw
an ArrayIndexOutOfBoundsException. Add a bounds check to prevent potential crashes
when the anchor's collation references fields outside the below-Project's output
range.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [275-280]

 private static boolean collationReferencesDerivedSlot(OpenSearchSort anchor, int[] belowProjOutToScan) {
     for (RelFieldCollation fc : anchor.getCollation().getFieldCollations()) {
-        if (belowProjOutToScan[fc.getFieldIndex()] < 0) return true;
+        int idx = fc.getFieldIndex();
+        if (idx >= belowProjOutToScan.length || belowProjOutToScan[idx] < 0) return true;
     }
     return false;
 }
Suggestion importance[1-10]: 7

__

Why: This is a valid safety concern. The method collationReferencesDerivedSlot accesses belowProjOutToScan[fc.getFieldIndex()] without bounds checking. If the anchor's collation references a field index beyond the array length, it would throw ArrayIndexOutOfBoundsException. Adding a bounds check prevents potential crashes.

Medium
General
Differentiate CPU and I/O cost scaling

The cost computation multiplies rows by widthFactor but uses the same value for both
CPU and I/O cost components. This may not accurately reflect the actual resource
consumption, as I/O cost should typically scale differently than CPU cost. Consider
using separate scaling factors or weighting the width factor differently for each
cost dimension.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rel/OpenSearchExchangeReducer.java [162-165]

 public RelOptCost computeSelfCost(RelOptPlanner planner, RelMetadataQuery mq) {
     double rows = mq.getRowCount(getInput());
     double widthFactor = getRowType().getFieldCount();
-    return planner.getCostFactory().makeCost(SETUP_COST + rows * widthFactor, SETUP_COST + rows * widthFactor, 0);
+    double cpuCost = SETUP_COST + rows * widthFactor;
+    double ioCost = SETUP_COST + rows * widthFactor * 0.5; // I/O scales less with width
+    return planner.getCostFactory().makeCost(cpuCost, ioCost, 0);
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion to differentiate CPU and I/O cost scaling is reasonable for cost modeling, but the specific scaling factor (0.5) is arbitrary. The current implementation treats both costs equally, which may be intentional. This is a minor optimization suggestion that could improve cost estimation accuracy.

Low
Verify field name uniqueness in mapping

The mapping relies on field names being unique and present in both scanFields and
wrapperFields. If a scan field name is missing from the wrapper (returns null from
the map), it's mapped to -1. However, if the wrapper contains duplicate field names,
the map will only retain the last occurrence, potentially causing incorrect index
mappings. Verify field name uniqueness or handle duplicates explicitly.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [429-433]

 int[] scanToWrapper = new int[scanFields.size()];
 for (int k = 0; k < scanFields.size(); k++) {
-    Integer w = wrapperIdxByName.get(scanFields.get(k).getName());
-    scanToWrapper[k] = (w == null) ? -1 : w;
+    String fieldName = scanFields.get(k).getName();
+    Integer w = wrapperIdxByName.get(fieldName);
+    if (w == null) {
+        scanToWrapper[k] = -1;
+    } else {
+        scanToWrapper[k] = w;
+    }
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion raises a valid concern about duplicate field names, but the improved_code doesn't actually address the issue—it's functionally identical to the existing_code. The concern about duplicate names in the wrapper is valid, but without actual verification logic or handling, this suggestion provides minimal value.

Low
Suggestions up to commit 5ee8f7f
CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate required helper fields presence

The nested loop searches for each helper field by name in childFields, but doesn't
verify that the helper was actually found. If a required helper like ___row_id or
___ugsi is missing from the child, the code silently skips it, potentially breaking
the trailing-helper layout contract that downstream execution stages depend on.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [579-588]

 for (String helper : OpenSearchLateMaterialization.TRAILING_HELPERS_IN_ORDER) {
+    boolean found = false;
     for (int i = 0; i < childFields.size(); i++) {
         if (helper.equals(childFields.get(i).getName())) {
             newProjects.add(new RexInputRef(i, childFields.get(i).getType()));
             newNames.add(helper);
+            found = true;
             break;
         }
     }
+    if (!found) {
+        throw new IllegalStateException("Required trailing helper '" + helper + "' not found in child fields");
+    }
 }
Suggestion importance[1-10]: 6

__

Why: Valid concern about silently skipping missing helper fields. The trailing helpers (___row_id, ___ugsi) are critical for the execution contract, and their absence would cause runtime failures. Adding validation here would catch configuration errors earlier, though the impact is moderate since the issue would surface during execution anyway.

Low
General
Clarify cost computation components

The cost computation multiplies rows by widthFactor but uses the same value for both
CPU and I/O cost components. This may not accurately reflect the actual resource
usage, as I/O cost should typically scale with data volume (rows × width) while CPU
cost might have different characteristics. Consider using distinct formulas for CPU
vs I/O, or document why they're identical.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rel/OpenSearchExchangeReducer.java [162-165]

 public RelOptCost computeSelfCost(RelOptPlanner planner, RelMetadataQuery mq) {
     double rows = mq.getRowCount(getInput());
     double widthFactor = getRowType().getFieldCount();
-    return planner.getCostFactory().makeCost(SETUP_COST + rows * widthFactor, SETUP_COST + rows * widthFactor, 0);
+    double ioCost = SETUP_COST + rows * widthFactor;
+    double cpuCost = SETUP_COST + rows * widthFactor;
+    return planner.getCostFactory().makeCost(ioCost, cpuCost, 0);
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies that CPU and I/O costs use identical formulas, but this is likely intentional for simplicity. The 'improved_code' merely adds intermediate variables without changing behavior, offering minimal value. The suggestion to "document why they're identical" would require adding comments, which scores 0 per guidelines.

Low
Suggestions up to commit a024e2e
CategorySuggestion                                                                                                                                    Impact
Possible issue
Add array bounds validation

Add bounds checking before array access to prevent ArrayIndexOutOfBoundsException.
The fc.getFieldIndex() could potentially exceed the belowProjOutToScan array length
if the collation references a field index beyond the below-Project's output schema.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [275-280]

 private static boolean collationReferencesDerivedSlot(OpenSearchSort anchor, int[] belowProjOutToScan) {
     for (RelFieldCollation fc : anchor.getCollation().getFieldCollations()) {
-        if (belowProjOutToScan[fc.getFieldIndex()] < 0) return true;
+        int idx = fc.getFieldIndex();
+        if (idx >= belowProjOutToScan.length || belowProjOutToScan[idx] < 0) return true;
     }
     return false;
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential ArrayIndexOutOfBoundsException if fc.getFieldIndex() exceeds the belowProjOutToScan array length. Adding bounds checking is a defensive programming practice that prevents runtime errors, though the likelihood depends on whether the collation indices are validated elsewhere in the codebase.

Medium
General
Handle duplicate field names correctly

The scanToWrapper mapping assumes all scan fields have unique names. If duplicate
field names exist in scanFields, the last occurrence will overwrite earlier mappings
in wrapperIdxByName, causing incorrect index resolution for earlier duplicates.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchLateMaterializationRewriter.java [429-433]

 int[] scanToWrapper = new int[scanFields.size()];
 for (int k = 0; k < scanFields.size(); k++) {
-    Integer w = wrapperIdxByName.get(scanFields.get(k).getName());
-    scanToWrapper[k] = (w == null) ? -1 : w;
+    String fieldName = scanFields.get(k).getName();
+    Integer w = wrapperIdxByName.get(fieldName);
+    if (w == null) {
+        scanToWrapper[k] = -1;
+    } else {
+        scanToWrapper[k] = w;
+        wrapperIdxByName.remove(fieldName);
+    }
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion raises a concern about duplicate field names, but the proposed solution (removing entries from wrapperIdxByName after first use) is flawed. The map is used to look up wrapper indices for multiple scan fields, and removing entries would break subsequent lookups. The original code's behavior (last occurrence wins) may be intentional or handled by schema validation elsewhere.

Low

…rite accordingly

Signed-off-by: Aniketh Jain <anijainc@amazon.com>
@expani
expani force-pushed the project_pushdown branch from 237ac65 to 5eda2f3 Compare June 9, 2026 12:18
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5eda2f3

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 5eda2f3: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Aniketh Jain <anijainc@amazon.com>
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 012994b

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 012994b: SUCCESS

@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.30%. Comparing base (723abcb) to head (0bff4dc).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #22082      +/-   ##
============================================
- Coverage     73.34%   73.30%   -0.05%     
- Complexity    75800    75806       +6     
============================================
  Files          6064     6064              
  Lines        344501   344498       -3     
  Branches      49575    49575              
============================================
- Hits         252670   252522     -148     
- Misses        71649    71859     +210     
+ Partials      20182    20117      -65     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b04ee2d

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for b04ee2d: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Aniketh Jain <anijainc@amazon.com>
@expani
expani force-pushed the project_pushdown branch from b04ee2d to d96b8dd Compare June 10, 2026 00:05
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5acbbbd

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 5acbbbd: SUCCESS

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0101411

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 0101411: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@sandeshkr419

Copy link
Copy Markdown
Member

@expani I think the sandbox errors are related to this change: https://github.com/opensearch-project/OpenSearch/actions/runs/27251790133/job/80483842313?pr=22082

Can you please check once if any tests need modifications.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 188161a

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 188161a: SUCCESS

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit a024e2e

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for a024e2e: SUCCESS

Signed-off-by: Aniketh Jain <anijainc@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5ee8f7f

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c573cb2

Signed-off-by: Aniketh Jain <anijainc@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 540429c

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 540429c: SUCCESS

expani and others added 3 commits June 12, 2026 18:22
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 9438fa9

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 9438fa9: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Aniketh Jain <anijainc@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0bff4dc

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 0bff4dc: SUCCESS

@sandeshkr419
sandeshkr419 merged commit 03283ee into opensearch-project:main Jun 13, 2026
16 checks passed
KhishorekumarBS pushed a commit to KhishorekumarBS/OpenSearch that referenced this pull request Jul 3, 2026
…ct#22082)

* Removed Sort Project Transpose rule and handled QTF detection and rewrite accordingly

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

* Fixed assertion caused by removal of Sort Project Transpose

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

* More changes to shape test because of Sort Project Transpose Removal

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

* Accounting for RowId and UGSI in FieldStorageInfo

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

* Added rule to duplicate 2 projects for percentile aggs to work

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

* Fixed the assertions for the new Rule

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

* Not dropping pure Project columns to ensure order is maintained

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

* Added plan shape tests for the new rule and fixed issue with TAKE

Signed-off-by: Aniketh Jain <anijainc@amazon.com>

---------

Signed-off-by: Aniketh Jain <anijainc@amazon.com>
Co-authored-by: Sandesh Kumar <sandeshkr419@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants