Skip to content

fix: validate recursive CTE column coercion - #25956

Open
goutamadwant wants to merge 2 commits into
apache:mainfrom
goutamadwant:fix/recursive-cte-lossy-coercion-25893
Open

goutamadwant wants to merge 2 commits into
apache:mainfrom
goutamadwant:fix/recursive-cte-lossy-coercion-25893

Conversation

@goutamadwant

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

A recursive CTE currently casts each recursive column to the non-recursive term's type, even when that discards information. A fractional increment can therefore become the same integer on every iteration, returning incorrect rows or preventing termination. The query should fail during planning and explain how to choose the intended output type.

What changes are included in this PR?

  • Validate each recursive column before the existing schema coercion, with an error that identifies the CTE, column, both types, and the need to cast the non-recursive term.
  • Preserve the integer-to-integer behavior added for CTE: Non-recursive term and recursive term should have the same schema #9794 and known lossless scalar widenings. Reject implicit casts that can discard numeric precision or narrow decimal range or scale.
  • Require matching compound types rather than inferring that nested casts are lossless; users can cast the non-recursive term to the intended compound type explicitly.
  • Keep the check in recursive-query planning, leaving the shared schema-coercion helper and execution path unchanged.

What is the testing strategy for this PR?

  • Added cte.slt regressions for float/decimal-to-integer coercion, integer-to-float precision loss above 2^53, float-to-decimal rounding, nested structs, cross-category coercion, and decimal range saturation. The error cases use EXPLAIN so they cannot run indefinitely.
  • Added positive cases for an explicitly widened seed and safe decimal/date/string/binary widening; the existing integer-to-integer recursion test still passes. The error cases were verified to fail without the relevant guard and pass with it.
  • Ran cargo fmt --all, cargo clippy --all-targets --all-features -- -D warnings, the focused cte.slt suite, and the extended workspace test command from AGENTS.md.

Are there any user-facing changes?

Yes. Recursive CTEs with unsafe implicit type coercion or mismatched compound types now fail at planning time with a hint to cast the non-recursive term to the intended type. No public API changes.

@github-actions github-actions Bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Oct 2, 2026
@codecov-commenter

codecov-commenter commented Oct 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.96721% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.89%. Comparing base (54e32bb) to head (fa17000).
⚠️ Report is 218 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/expr/src/logical_plan/builder.rs 81.96% 10 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25956      +/-   ##
==========================================
+ Coverage   82.58%   82.89%   +0.30%     
==========================================
  Files        1144     1154      +10     
  Lines      443198   458083   +14885     
  Branches   443198   458083   +14885     
==========================================
+ Hits       366024   379726   +13702     
- Misses      54978    55317     +339     
- Partials    22196    23040     +844     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kumarUjjawal kumarUjjawal 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.

Thank you @goutamadwant for the fix.

Comment on lines +123 to +125
if type_union_coercion(into, from).as_ref() != Some(into) {
return false;
}

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.

Medium · Apply decimal range checks before the UNION-type gate

For an Int64 recursive column and a DECIMAL(19,0) seed, this gate rejects a lossless conversion: type_union_coercion represents signed and unsigned 64-bit integers as Decimal128(20,0), although every signed Int64 fits in 19 digits. Consequently, WITH RECURSIVE t AS (SELECT 1::DECIMAL(19,0) AS a UNION ALL SELECT 2::BIGINT FROM t WHERE a < 2) SELECT * FROM t now fails planning. The more accurate signed-integer range check below is never reached. Handle decimal destinations with that range/scale check before requiring equality with UNION's common type.

@kumarUjjawal kumarUjjawal 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.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recursive CTE silently casts recursive term to the static term's type, producing wrong results or a non-terminating query

3 participants