[multistage] Enable Tests for New Optimizer + Bug Fixes by ankitsultana · Pull Request #15958 · apache/pinot

ankitsultana · 2025-05-31T08:01:56Z

Summary

Adds the required fixes to make all tests in ResourceBasedQueriesTest pass with the new optimizer (both with and without lite mode).

Bugs Fixed / Gaps Addressed

Ensures Window groups are converted to RexLiteral. Logic is same as PinotWindowExchangeNodeInsertRule, and I have abstracted out the common code to a utils class. It's not a super clean approach code-style wise and I am open to feedback here.
Added support for As Of Join.
Fixed some trait assignment code.
Fixes AggregatePushdownRule to allow it to find Projects under Exchange. This was causing failure of some complex agg functions due to incorrect agg call generation.

Test Plan

Added E2E tests in ResourceBasedQueriesTest.

ankitsultana · 2025-06-10T20:10:26Z

pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotRuleUtils.java

    return funcSqlKind == SqlKind.OTHER_FUNCTION ? function.getOperator().getName() : funcSqlKind.name();
  }
+
+  public static class WindowUtils {


note: lifted as is from the window exchange rule

wirybeaver · 2025-06-11T13:37:54Z

pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotRuleUtils.java

+     * NOTE: {@link Window} has a field called "constants" which contains the literal values. If the input reference is
+     * beyond the window input size, it is a reference to the constants.
+     */
+    public static Window.Group updateLiteralArgumentsInWindowGroup(Window window) {


Could you add more comments about how does RexInputRef(x) find the element in constant list with more details? That's what I get by using copilot.

1. Understanding the Inputs
When Calcite constructs a Window relational node, it conceptually sees the input as:
All columns from the input relation (e.g., your table columns)
Followed by any literal constants needed by window functions (these are placed in the constants list in the Window node)
So, if your input row has N columns, and your window function needs K constants, the total "input" to the Window node is N + K.
2. How RexInputRef Works in This Context
RexInputRef(i) refers to the i-th field in the Window node's input.
If i < N, it points to the i-th field of the original input row (e.g., a table column).
If i >= N, it points to the (i - N)-th entry in the constants list.
Example:
Suppose your table has 2 columns: employee_id, salary.
Suppose your window function is:
SQL
LAG(salary, 2, 0) OVER (PARTITION BY dept ORDER BY hire_date)
The constants needed are 2 and 0, so constants = [2, 0].
Input to the Window node:
employee_id | salary | 2 | 0 -- | -- | -- | -- ... | ... | 2 | 0
So:
RexInputRef(0) → employee_id
RexInputRef(1) → salary
RexInputRef(2) → constants[0] = 2
RexInputRef(3) → constants[1] = 0

This is lifted as is from PinotWindowExchangeNodeInsertRule. GPT response is largely right: rex input ref is an integer reference to the input, but in window group it could exceed the field count of the input, in which case it is actually a reference to window.constants.

wirybeaver · 2025-06-11T15:28:30Z

pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotRuleUtils.java

+      }
+      RexWindowBound upperBound = oldWindowGroup.upperBound;
+      offset = upperBound.getOffset();
+      if (offset != null) {


Add comment on when the offset is null; unbounded preceding / unbounded following / current row

Lifted as is from PinotWindowExchangeNodeInsertRule. I suppose offset is not null when functions like Lead and Lag are used.

wirybeaver · 2025-06-11T15:31:09Z

pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotRuleUtils.java

+          && !upperBound.isUnbounded());
+
+      if (!windowGroup.isRows) {
+        Preconditions.checkState(!hasOffset, "RANGE window frame with offset PRECEDING / FOLLOWING is not supported");


https://docs.pinot.apache.org/windows-functions The range frame clause with offset is claimed to be supported in the doc. Do I miss the content?

codecov-commenter · 2025-06-16T18:17:15Z

Codecov Report

❌ Patch coverage is 89.06250% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.18%. Comparing base (1a476de) to head (1a2ae28).
⚠️ Report is 1291 commits behind head on master.

Files with missing lines	Patch %	Lines
...apache/pinot/calcite/rel/rules/PinotRuleUtils.java	87.83%	5 Missing and 4 partials ⚠️
...y/planner/physical/v2/PRelToPlanNodeConverter.java	77.77%	0 Missing and 4 partials ⚠️
.../query/planner/physical/v2/RelToPRelConverter.java	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #15958      +/-   ##
============================================
+ Coverage     62.90%   63.18%   +0.28%     
+ Complexity     1386     1355      -31     
============================================
  Files          2867     2950      +83     
  Lines        163354   169370    +6016     
  Branches      24952    25896     +944     
============================================
+ Hits         102755   107016    +4261     
- Misses        52847    54223    +1376     
- Partials       7752     8131     +379

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`?`
java-11	`63.16% <89.06%> (+0.29%)`	⬆️
java-21	`63.15% <89.06%> (+0.33%)`	⬆️
skip-bytebuffers-false	`?`
skip-bytebuffers-true	`?`
temurin	`63.18% <89.06%> (+0.28%)`	⬆️
unittests	`63.18% <89.06%> (+0.28%)`	⬆️
unittests1	`56.33% <89.06%> (+0.51%)`	⬆️
unittests2	`33.29% <0.00%> (-0.29%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

* [multistage] Enable Tests for New Optimizer + Bug Fixes * all tests pass * fix checkstyle

Jackie-Jiang · 2025-06-27T19:02:52Z

Seems the new added tests are flaky. @ankitsultana Can you please take a look?
Example run: https://github.com/apache/pinot/actions/runs/15924989688/job/44920115015?pr=16216

ankitsultana · 2025-06-27T21:26:14Z

Looking into it and created #16223

ankitsultana added mse-physical-optimizer multi-stage Related to the multi-stage query engine labels May 31, 2025

This was referenced Jun 5, 2025

[multistage] Lite Mode with Scatter Gather Execution #16000

Merged

[multistage] Handle Column Less Tuples in BlockSplitter #16058

Merged

ankitsultana force-pushed the mse-physical-tests branch from 6158e13 to 977ff75 Compare June 10, 2025 19:59

ankitsultana changed the title ~~[do-not-merge] [multistage] Enable Tests for New Optimizer + Bug Fixes~~ [multistage] Enable Tests for New Optimizer + Bug Fixes Jun 10, 2025

ankitsultana commented Jun 10, 2025

View reviewed changes

ankitsultana requested a review from itschrispeck June 10, 2025 20:11

ankitsultana marked this pull request as ready for review June 10, 2025 20:11

wirybeaver reviewed Jun 11, 2025

View reviewed changes

shauryachats approved these changes Jun 12, 2025

View reviewed changes

ankitsultana added 3 commits June 16, 2025 16:45

[multistage] Enable Tests for New Optimizer + Bug Fixes

c86d252

all tests pass

255cb0a

fix checkstyle

1a2ae28

ankitsultana force-pushed the mse-physical-tests branch from 20e545e to 1a2ae28 Compare June 16, 2025 17:34

itschrispeck approved these changes Jun 16, 2025

View reviewed changes

itschrispeck merged commit 7dbbc09 into apache:master Jun 16, 2025
32 of 36 checks passed

songwdfu pushed a commit to songwdfu/pinot that referenced this pull request Jun 18, 2025

[multistage] Enable Tests for New Optimizer + Bug Fixes (apache#15958)

ced630e

* [multistage] Enable Tests for New Optimizer + Bug Fixes * all tests pass * fix checkstyle

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multistage] Enable Tests for New Optimizer + Bug Fixes#15958

[multistage] Enable Tests for New Optimizer + Bug Fixes#15958
itschrispeck merged 3 commits intoapache:masterfrom
ankitsultana:mse-physical-tests

ankitsultana commented May 31, 2025 •

edited

Loading

Uh oh!

ankitsultana Jun 10, 2025

Uh oh!

wirybeaver Jun 11, 2025

Uh oh!

ankitsultana Jun 11, 2025

Uh oh!

wirybeaver Jun 11, 2025

Uh oh!

ankitsultana Jun 11, 2025

Uh oh!

wirybeaver Jun 11, 2025

Uh oh!

ankitsultana Jun 11, 2025

Uh oh!

wirybeaver Jun 12, 2025

Uh oh!

codecov-commenter commented Jun 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

Jackie-Jiang commented Jun 27, 2025

Uh oh!

ankitsultana commented Jun 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

ankitsultana commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Bugs Fixed / Gaps Addressed

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

1. Understanding the Inputs

2. How RexInputRef Works in This Context

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Jackie-Jiang commented Jun 27, 2025

Uh oh!

ankitsultana commented Jun 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ankitsultana commented May 31, 2025 •

edited

Loading

codecov-commenter commented Jun 16, 2025 •

edited

Loading