You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "discord9 (via GitHub)" <gi...@apache.org> on 2026/04/29 03:25:52 UTC
[PR] fix: forbid lossy cast(for now) [datafusion]
discord9 opened a new pull request, #21908:
URL: https://github.com/apache/datafusion/pull/21908
## Which issue does this PR close?
## Rationale for this change
as title, forbid lossy cast for now(maybe could add them back as between expr later?) like cast(ts_ns to ms) == literal_ms, which is not equal to ts_ns == literal_ns since like cast(1ns)=0ms but 1ns!=0ns
## What changes are included in this PR?
simply forbid lossy cast for now in ExprSimpilfy
## Are these changes tested?
unit tests are added
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
this is a fix, so I guess previous incorrect behavior will be change?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: forbid lossy cast(for now) [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on PR #21908:
URL: https://github.com/apache/datafusion/pull/21908#issuecomment-4417285911
@Jefffrey added slt test now, this pr might seem a little large now, but it's mostly tests and simple conservative judgment criteria and should be safe
--
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: forbid lossy cast(for now) [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on PR #21908:
URL: https://github.com/apache/datafusion/pull/21908#issuecomment-4436688712
> Thanks for this contribution!
>
> As @Jefffrey suggests, this PR would be easier to review if you clarified the behavior you want to change. For example, "forbid lossy cast" is quite vague about specifically which casts were previously allowed, why those casts are lossy, and which casts will now be disallowed as a result of this PR. It would also be nice to create a separate GitHub issue, as the PR template requests.
>
> I wonder whether we want closed-by-default or open-by-default behavior. That is, instead of this approach, we could start by assuming that all casts are unsafe and then enumerate the specific casts that we want to allow. That would be safer, e.g., in the event of new types or new casts being added. Implementation might be simpler as well.
sounds reasonable, how about for now only allow downcast like optimize `cast(ts_ms)=lit_ns` to `ts_ms==lit_ms`, does sound much safer and feels like the original intend
--
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: forbid lossy cast(for now) [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #21908:
URL: https://github.com/apache/datafusion/pull/21908#discussion_r3228119287
##########
datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs:
##########
@@ -59,46 +59,43 @@ use datafusion_common::{Result, ScalarValue};
use datafusion_common::{internal_err, tree_node::Transformed};
use datafusion_expr::{BinaryExpr, lit};
use datafusion_expr::{Cast, Expr, Operator, TryCast, simplify::SimplifyContext};
-use datafusion_expr_common::casts::{is_supported_type, try_cast_literal_to_type};
+use datafusion_expr_common::casts::{
+ CastComparisonRewrite, is_supported_type, try_cast_literal_for_comparison_unwrap,
+ try_cast_literal_to_type,
+};
pub(super) fn unwrap_cast_in_comparison_for_binary(
info: &SimplifyContext,
- cast_expr: Expr,
- literal: Expr,
+ cast_expr: &Expr,
+ literal: &Expr,
op: Operator,
) -> Result<Transformed<Expr>> {
+ let original = Expr::BinaryExpr(BinaryExpr {
Review Comment:
Doing two clones eagerly here seems unnecessary, especially for the no-op path; we can move this into where `original` is actually used.
--
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: replace default-allow cast unwrap with closed-by-default allowlist (timestamp + integer widening) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #21908:
URL: https://github.com/apache/datafusion/pull/21908#issuecomment-4606677178
> The previous unwrap_cast optimization was default-allow: it would move the cast from the
expression side to the literal side for virtually any CAST(column AS T) <op> literal_T pattern.
This is semantically unsafe for many casts — the rewritten predicate can match different rows
than the original.
I am still missing what problem we are solving
Is the root cause this issue in Greptime?
- https://github.com/GreptimeTeam/greptimedb/issues/8214
If so, perhaps we can do a more targeted fix for that issue first to resolve the bug, and then we can contemplate a larger scale refactor of the code as a follow on.
Right now I fear we are blocking a bug fix behind a very large code code change, that might be good on its own, may also have non trivial knock on effects
--
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: replace default-allow cast unwrap with closed-by-default allowlist (timestamp + integer widening) [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on PR #21908:
URL: https://github.com/apache/datafusion/pull/21908#issuecomment-4678663379
Close for maybe using udf_preimage rewrite machinery for unwrap_cast
--
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: replace default-allow cast unwrap with closed-by-default allowlist (timestamp + integer widening) [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on PR #21908:
URL: https://github.com/apache/datafusion/pull/21908#issuecomment-4459439590
add a lot of allow list to avoid too much regression
--
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: forbid lossy cast(for now) [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on PR #21908:
URL: https://github.com/apache/datafusion/pull/21908#issuecomment-4341298371
> Is there an end-to-end example (e.g. SLT) that showcases the bug being fixed here?
will add now, althought I just realize that int 32->int16 could also be lossy cast so need fix too
--
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: replace default-allow cast unwrap with closed-by-default allowlist (timestamp + integer widening) [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on PR #21908:
URL: https://github.com/apache/datafusion/pull/21908#issuecomment-4608626695
> > The previous unwrap_cast optimization was default-allow: it would move the cast from the
> > expression side to the literal side for virtually any CAST(column AS T) literal_T pattern.
> > This is semantically unsafe for many casts — the rewritten predicate can match different rows
> > than the original.
>
> I am still missing what problem we are solving
>
> Is the root cause this issue in Greptime?
>
> * [TIMESTAMP predicate incorrectly matches rows with higher fractional seconds GreptimeTeam/greptimedb#8214](https://github.com/GreptimeTeam/greptimedb/issues/8214)
>
>
> If so, perhaps we can do a more targeted fix for that issue first to resolve the bug, and then we can contemplate a larger scale refactor of the code as a follow on.
>
> Right now I fear we are blocking a bug fix behind a very large code code change, that might be good on its own, may also have non trivial knock on effects
so this fix start exactly as you suggest a small blocklist(see 565eed2231dfff96bfecdaba3613df73f3d5d69e) but as reviewer sugest a allowlist would be better, it was later changed to a allowlist, mabe we cn make a new pr for a quickfix for smaller blcoklist menawhile having enough tme to describe a proper allowlist and even more sophisticated optimize?
--
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: replace default-allow cast unwrap with closed-by-default allowlist (timestamp + integer widening) [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 closed pull request #21908: fix: replace default-allow cast unwrap with closed-by-default allowlist (timestamp + integer widening)
URL: https://github.com/apache/datafusion/pull/21908
--
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