You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Jamison929611 (via GitHub)" <gi...@apache.org> on 2026/08/12 09:23:08 UTC
[PR] feat: add implicit native coercion constructor [datafusion]
Jamison929611 opened a new pull request, #24282:
URL: https://github.com/apache/datafusion/pull/24282
## Which issue does this PR close?
- Closes #24135.
## Rationale for this change
When a coercion's desired type is a native logical type, callers currently have to repeat that type as both `TypeSignatureClass::Native(...)` and `default_casted_type`. This duplication is confusing and can allow the two values to disagree.
## What changes are included in this PR?
- Add `Coercion::new_implicit_native`, which derives `default_casted_type` from the desired logical type and delegates to the existing `new_implicit` constructor.
- Keep `Coercion::new_implicit` unchanged for backward compatibility and non-native desired type classes.
- Migrate the representative `reverse` and `approx_percentile_cont` call sites linked from the issue.
- Update the `Coercion` rustdoc example and add a constructor-equivalence unit test.
## Are these changes tested?
Yes:
- `cargo test -p datafusion-expr-common test_new_implicit_native --lib`
- `cargo test -p datafusion-functions reverse --lib`
- `cargo test -p datafusion-functions-aggregate approx_percentile_cont --lib`
- `cargo test -p datafusion-expr-common --doc`
- `cargo clippy -p datafusion-expr-common -p datafusion-functions -p datafusion-functions-aggregate --all-targets --all-features -- -D warnings`
- `cargo fmt --all`
The required full-workspace `cargo clippy --all-targets --all-features -- -D warnings` was also attempted on Windows. It reached an unrelated MSVC native-link failure while building `protobuf-src` (`LNK2005`/`LNK4098` in `protoc-gen-upb`); affected-crate clippy passed with warnings denied, and CI will run the full Linux workspace checks.
No benchmark was run because this is an API convenience constructor that delegates to the existing constructor and does not change runtime execution.
## Are there any user-facing changes?
Yes. This adds the public `Coercion::new_implicit_native` convenience API. It is additive and backward compatible; existing `new_implicit` callers continue to work unchanged.
--
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] feat: add implicit native coercion constructor [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24282:
URL: https://github.com/apache/datafusion/pull/24282#issuecomment-5271038148
FYI @Jefffrey you may be interested in this PR
--
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] feat: add implicit native coercion constructor [datafusion]
Posted by "Jamison929611 (via GitHub)" <gi...@apache.org>.
Jamison929611 commented on PR #24282:
URL: https://github.com/apache/datafusion/pull/24282#issuecomment-5264881784
The substantive GitHub Actions workflows are currently marked `action_required` pending maintainer approval for this fork contributor. When convenient, could a maintainer approve the workflow runs? I'll monitor and address any resulting failures or review feedback.
--
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] feat: add implicit native coercion constructor [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #24282:
URL: https://github.com/apache/datafusion/pull/24282
--
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] feat: add implicit native coercion constructor [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #24282:
URL: https://github.com/apache/datafusion/pull/24282#issuecomment-5306253337
thanks @Jamison929611 & @jayzhan211
--
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] feat: add implicit native coercion constructor [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24282:
URL: https://github.com/apache/datafusion/pull/24282#issuecomment-5271353233
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24282?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 81.28%. Comparing base ([`c08832d`](https://app.codecov.io/gh/apache/datafusion/commit/c08832d481cea2dcea98e43393e3dd640d421064?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`524966a`](https://app.codecov.io/gh/apache/datafusion/commit/524966ac7d574680ec9b616b3606f71a02b03cf9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 5 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24282 +/- ##
==========================================
- Coverage 81.29% 81.28% -0.01%
==========================================
Files 1110 1110
Lines 385205 385223 +18
Branches 385205 385223 +18
==========================================
+ Hits 313145 313147 +2
- Misses 53580 53593 +13
- Partials 18480 18483 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24282?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] feat: add implicit native coercion constructor [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #24282:
URL: https://github.com/apache/datafusion/pull/24282#discussion_r3781426251
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -1118,6 +1117,22 @@ impl Coercion {
}
}
+ /// Create a new coercion to a native logical type with implicit coercion rules.
+ ///
+ /// The native type of `desired_type` is used as the default type when coercing
+ /// from `allowed_source_types`.
Review Comment:
could we add a note to this documentation to explain why one would prefer using this over `new_implicit()`
e.g. "convenience method when coercing to a specific logical 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
Re: [PR] feat: add implicit native coercion constructor [datafusion]
Posted by "Jamison929611 (via GitHub)" <gi...@apache.org>.
Jamison929611 commented on code in PR #24282:
URL: https://github.com/apache/datafusion/pull/24282#discussion_r3782572311
##########
datafusion/expr-common/src/signature.rs:
##########
@@ -1118,6 +1117,22 @@ impl Coercion {
}
}
+ /// Create a new coercion to a native logical type with implicit coercion rules.
+ ///
+ /// The native type of `desired_type` is used as the default type when coercing
+ /// from `allowed_source_types`.
Review Comment:
Updated the docs to call this out as a convenience method for coercing to a specific logical type and avoiding passing the native default cast type separately.
--
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