Skip to content

Fix CAST type in multi-stage EXPLAIN when asking servers - #19768

Merged
yashmayya merged 2 commits into
apache:masterfrom
yashmayya:explain-plan-cast
Oct 7, 2026
Merged

yashmayya merged 2 commits into
apache:masterfrom
yashmayya:explain-plan-cast

Conversation

@yashmayya

@yashmayya yashmayya commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

PR flow

PR modifies toRexCall to correctly build Calcite CAST from operand and target type. (5 nodes, 5 edges)

flowchart TD
  N0["toRexCall handles CAST #40;F1#41;"]:::stModified
  N1["Extract first operand #40;F1#41;"]:::stAdded
  N2["Get RelDataTypeFactory #40;F1#41;"]:::stAdded
  N3["Create target type with nullability #40;F1#41;"]:::stAdded
  N4["Make Calcite cast #40;F1#41;"]:::stAdded
  N0 -->|"calls"| N1
  N0 -->|"calls"| N2
  N1 -->|"provides operand"| N4
  N2 -->|"provides factory"| N3
  N3 -->|"provides type"| N4
  classDef stAdded fill:#dafbe1,stroke:#1a7f37,color:#1f2328,stroke-width:2px
  classDef stModified fill:#fff8c5,stroke:#9a6700,color:#1f2328,stroke-width:2px
  classDef stRemoved fill:#ffebe9,stroke:#cf222e,color:#1f2328,stroke-width:2px
  classDef stUnchanged fill:#f6f8fa,stroke:#656d76,color:#1f2328,stroke-width:1px
Loading

AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing

Diff evidence
  • F1: pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java — before · after
  • Regenerate PR flow

This PR fixes the CAST item in the "Limits" section of #19766.

The bug

With explainAskingServers (or pinot.query.multistage.explain.include.segment.plan), PlanNodeToRelConverter converts the plan nodes from the servers back to Calcite nodes.

A plan node holds CAST(x AS T) as a CAST call with two operands: x, and a STRING literal with the name of T (see RexExpressionUtils.handleCast()). toRexCall() gives both operands to the Calcite CAST operator. Calcite then uses the type of the second operand as the type of the cast.

For example, take this query with the default planner rules:

SELECT Carrier, SUM(v) FROM (SELECT /*+ aggOptions(is_partitioned_by_group_by_keys='true') */ Carrier, Origin, AVG(ArrDelay) AS v FROM mytable GROUP BY Carrier, Origin) GROUP BY Carrier

EXPLAIN shows:

LogicalProject(Carrier=[$0], $f1=[DIVIDE(CAST($2, 'DOUBLE'):CHAR(6) NOT NULL, $3)])

Before #19766, the nodes with this CAST usually showed as Unknown, so this bug was hidden.

The fix

In toRexCall(), create CAST with RexBuilder.makeCast() from the first operand. Use the type of the call as the target type, with the nullability of the operand. This is the inverse of handleCast(). EXPLAIN now shows:

LogicalProject(Carrier=[$0], $f1=[DIVIDE(CAST($2):DOUBLE NOT NULL, $3)])

Only the EXPLAIN output changes. Query execution does not use this conversion.

Testing

  • RexExpressionUtilsTest.testCastRoundTrip converts a CAST call to a Calcite node and back. It also checks that a nullable operand gives a nullable cast. Without this change, the test fails in handleCast(), which expects one operand.
  • I ran the query above in a temporary integration test with pinot.query.multistage.explain.include.segment.plan=true and the default planner rules. EXPLAIN shows DIVIDE(CAST($2):DOUBLE NOT NULL, $3) and no Unknown nodes.
  • All 2659 pinot-query-planner tests pass. spotless, checkstyle and license are clean.

@yashmayya yashmayya added bug Something is not working as expected multi-stage Related to the multi-stage query engine labels Oct 6, 2026
@yashmayya
yashmayya requested a review from gortiz October 6, 2026 18:14
@codecov-commenter

codecov-commenter commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.40%. Comparing base (73645da) to head (81a9779).

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19768      +/-   ##
============================================
- Coverage     68.40%   68.40%   -0.01%     
  Complexity     1470     1470              
============================================
  Files          3521     3521              
  Lines        229091   229097       +6     
  Branches      36346    36347       +1     
============================================
- Hits         156707   156703       -4     
+ Misses        60104    60101       -3     
- Partials      12280    12293      +13     
Flag Coverage Δ
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 68.40% <100.00%> (-0.01%) ⬇️
lane-a 100.00% <ø> (ø)
lane-b 0.00% <ø> (ø)
temurin 68.40% <100.00%> (-0.01%) ⬇️
unittests 68.39% <100.00%> (-0.01%) ⬇️
unittests1 58.20% <100.00%> (+<0.01%) ⬆️
unittests2 40.04% <0.00%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Harness.
📢 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.

@yashmayya
yashmayya requested a review from Jackie-Jiang October 6, 2026 22:34

@Jackie-Jiang Jackie-Jiang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the CAST-fix commit 046f050, excluding dependency #19766. The target type and operand nullability are reconstructed correctly, and the round-trip test covers the reported regression. LGTM, with one minor assertion-import nit.

All 13 reported CI checks pass for this commit. I did not run tests locally.

Comment on lines +101 to +102
Assert.assertTrue(rexNode.getType().isNullable());
Assert.assertEquals(RexExpressionUtils.fromRexNode(rexNode), castCall);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Please use static imports for assertTrue and assertEquals in the new test, following the test assertion convention.

@yashmayya
yashmayya merged commit 3de4f31 into apache:master Oct 7, 2026
20 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working as expected multi-stage Related to the multi-stage query engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants