You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "goutamadwant (via GitHub)" <gi...@apache.org> on 2026/10/02 02:52:53 UTC
[PR] fix: validate recursive CTE column coercion [datafusion]
goutamadwant opened a new pull request, #25956:
URL: https://github.com/apache/datafusion/pull/25956
## Which issue does this PR close?
- Closes #25893.
## 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 #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate recursive CTE column coercion [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #25956:
URL: https://github.com/apache/datafusion/pull/25956
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate recursive CTE column coercion [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25956:
URL: https://github.com/apache/datafusion/pull/25956#issuecomment-5944983402
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25956?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `80.32787%` with `12 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.59%. Comparing base ([`54e32bb`](https://app.codecov.io/gh/apache/datafusion/commit/54e32bb7e5c4d9d35d801b4cec5fab8c010a4490?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`62908ef`](https://app.codecov.io/gh/apache/datafusion/commit/62908effd8894cb6e6a24b7bcaef92bbc8592c1f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 15 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25956?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/expr/src/logical\_plan/builder.rs](https://app.codecov.io/gh/apache/datafusion/pull/25956?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Flogical_plan%2Fbuilder.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9sb2dpY2FsX3BsYW4vYnVpbGRlci5ycw==) | 80.32% | [11 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25956?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25956 +/- ##
========================================
Coverage 82.58% 82.59%
========================================
Files 1144 1145 +1
Lines 443198 443993 +795
Branches 443198 443993 +795
========================================
+ Hits 366024 366707 +683
- Misses 54978 55025 +47
- Partials 22196 22261 +65
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25956?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate recursive CTE column coercion [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #25956:
URL: https://github.com/apache/datafusion/pull/25956#discussion_r4163107479
##########
datafusion/expr/src/logical_plan/builder.rs:
##########
@@ -88,6 +90,93 @@ impl LogicalPlanBuilderOptions {
}
}
+fn recursive_type_can_be_coerced(from: &DataType, into: &DataType) -> bool {
+ if from == into || *from == DataType::Null {
+ return true;
+ }
+ // Preserve the integer coercion supported by recursive queries since #9794.
+ if from.is_integer() && into.is_integer() {
+ return true;
+ }
+ if matches!(
+ (from, into),
+ (DataType::Date32, DataType::Date64)
+ | (DataType::Utf8, DataType::LargeUtf8 | DataType::Utf8View)
+ | (DataType::LargeUtf8, DataType::Utf8View)
+ | (
+ DataType::Binary,
+ DataType::LargeBinary | DataType::BinaryView
+ )
+ | (DataType::LargeBinary, DataType::BinaryView)
+ | (
+ DataType::FixedSizeBinary(_),
+ DataType::Binary | DataType::BinaryView
+ )
+ ) {
+ return true;
+ }
+ // Compound types and changes of type category need an explicit seed type.
+ // UNION's common-type rules can hide lossy casts within nested fields.
+ if !from.is_numeric() || !into.is_numeric() {
+ return false;
+ }
+ if type_union_coercion(into, from).as_ref() != Some(into) {
+ return false;
+ }
Review Comment:
**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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org