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 2025/04/25 09:27:38 UTC
[PR] fix: fold cast null to typed null [datafusion]
discord9 opened a new pull request, #15854:
URL: https://github.com/apache/datafusion/pull/15854
## Which issue does this PR close?
TODO(file a issue later)
- Closes #.
## Rationale for this change
fix a error where `SELECT NULL::DOUBLE` can't be encode to substrait
## What changes are included in this PR?
replace `cast(NULL, <data type>` to typed null in substrait
## Are these changes tested?
WIP
## Are there any user-facing changes?
shouldn't be any, simply a constant folding shouldn't affect downstream system
--
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: fold cast null to substrait typed null [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#discussion_r2067014140
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -1590,6 +1590,21 @@ pub fn from_cast(
schema: &DFSchemaRef,
) -> Result<Expression> {
let Cast { expr, data_type } = cast;
+ // since substrait Null must be typed, so if we see a cast(null, dt), we make it a typed null
+ if let Expr::Literal(lit) = expr.as_ref() {
+ if lit.is_null() {
Review Comment:
To further clarify, at the SQL/Relational level
```sql
SELECT null::text[], array[]::text[]
text | array
---- | --
null | {}
```
we can distinguish between casting a null literal to a compound type, and the empty version of that compound type.
[dbfiddle](https://www.db-fiddle.com/f/cH7Hwk6mHnm3rgy7wXG4Yq/47)
--
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: fold cast null to substrait typed null [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#discussion_r2069115514
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -1590,6 +1590,21 @@ pub fn from_cast(
schema: &DFSchemaRef,
) -> Result<Expression> {
let Cast { expr, data_type } = cast;
+ // since substrait Null must be typed, so if we see a cast(null, dt), we make it a typed null
+ if let Expr::Literal(lit) = expr.as_ref() {
+ if lit.is_null() {
Review Comment:
I don't really grok the `is_null` logic for compound types super well.
I did test locally with
```rust
let sv = ScalarValue::List(Arc::new(GenericListArray::new_null(
Field::new_list_field(DataType::Float32, true).into(),
1,
)));
let target_type = DataType::List(
Field::new_list_field(DataType::Int32, true).into()
);
let expr = Expr::Literal(sv)
.cast_to(&target_type, &empty_schema)
.unwrap();
```
and was able to trigger the folding behaviour, which is undesirable... I think?
@discord9 it might be good to capture that we _shouldn't_ fold this as a test.
--
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: fold cast null to substrait typed null [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#issuecomment-2852002030
Thanks again @discord9 @vbarua and @gabotechs
--
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: fold cast null to substrait typed null [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#discussion_r2067003785
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -1590,6 +1590,21 @@ pub fn from_cast(
schema: &DFSchemaRef,
) -> Result<Expression> {
let Cast { expr, data_type } = cast;
+ // since substrait Null must be typed, so if we see a cast(null, dt), we make it a typed null
+ if let Expr::Literal(lit) = expr.as_ref() {
+ if lit.is_null() {
Review Comment:
The one thing I'm not sure about is the usage of `is_null()` here. I think the logic inside that works for non-compound types here:
```rust
pub fn is_null(&self) -> bool {
match self {
ScalarValue::Boolean(v) => v.is_none(),
ScalarValue::Null => true,
ScalarValue::Float16(v) => v.is_none(),
```
But for compound types I'm not sure if it does the right thing for the fold you're introducing:
```rust
ScalarValue::List(arr) => arr.len() == arr.null_count(),
ScalarValue::LargeList(arr) => arr.len() == arr.null_count(),
ScalarValue::FixedSizeList(arr) => arr.len() == arr.null_count(),
ScalarValue::Struct(arr) => arr.len() == arr.null_count(),
ScalarValue::Map(arr) => arr.len() == arr.null_count(),
```
because I don't think that it make sense to flatten empty compound types. If I understand how this would work, something like
```
Cast(List[], List<Int32>)
```
would get flattened into a null literal of type `List<i32>` which is subtly wrong, because it should be a empty `List<i32>`.
--
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: fold cast null to substrait typed null [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#issuecomment-2841594671
@vbarua might want to take a look again whether this fix is right
--
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: fold cast null to substrait typed null [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#issuecomment-2841844822
I think the CI tests will pass once we merge up from main
--
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: fold cast null to substrait typed null [datafusion]
Posted by "Blizzara (via GitHub)" <gi...@apache.org>.
Blizzara commented on code in PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#discussion_r2068664824
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -1590,6 +1590,21 @@ pub fn from_cast(
schema: &DFSchemaRef,
) -> Result<Expression> {
let Cast { expr, data_type } = cast;
+ // since substrait Null must be typed, so if we see a cast(null, dt), we make it a typed null
+ if let Expr::Literal(lit) = expr.as_ref() {
+ if lit.is_null() {
Review Comment:
@vbarua re
> But for compound types I'm not sure if it does the right thing for the fold you're introducing:
> because I don't think that it make sense to flatten empty compound types. If I understand how this would work, something like
There is a comment in the code of "is_null" before those:
> // arr.len() should be 1 for a list scalar, but we don't seem to
> // enforce that anywhere, so we still check against array length.
I haven't validated if that is correct, but I guess the idea is that a SV:List is null if the _outer, "one-row" list itself_ is null. If that list is not null, then the inner list can be empty. So an empty compound type would have first the `arr` with `arr.len() = 1, and `arr.null_count() = 0`, passing the check; then arr.values(0) would be another list which can have its own length and null count (where e.g. both would then be zero for an empty list).
So unless I misunderstood what you meant, or there's some bug that I'm not aware of / or a difference in the behavior to what I think it is, I do believe the earlier code was also correct. However the change to only deal with SV:Null seems good to me as well.
(I do find it annoying how complicated the compound types for SV are to understand. Why is SV::List a single-row ListArray instead of just a Vec<ScalarValue> ? But maybe there's a reason, and in any case changing that is probably too much of a hassle now, and totally outside the scope of 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] fix: fold cast null to substrait typed null [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #15854:
URL: https://github.com/apache/datafusion/pull/15854
--
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: fold cast null to substrait typed null [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on code in PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#discussion_r2067801065
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -1590,6 +1590,21 @@ pub fn from_cast(
schema: &DFSchemaRef,
) -> Result<Expression> {
let Cast { expr, data_type } = cast;
+ // since substrait Null must be typed, so if we see a cast(null, dt), we make it a typed null
+ if let Expr::Literal(lit) = expr.as_ref() {
+ if lit.is_null() {
Review Comment:
> To further clarify, at the SQL/Relational level
>
> ```sql
> SELECT null::text[], array[]::text[]
>
> text | array
> ---- | --
> null | {}
> ```
>
> we can distinguish between casting a null literal to a compound type, and the empty version of that compound type.
>
> [dbfiddle](https://www.db-fiddle.com/f/cH7Hwk6mHnm3rgy7wXG4Yq/47)
Oh I didn't realize `is_null` do extra things other than matches with `ScalarValue::Null`, I shall only match `ScalarValue::Null` then
--
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: fold cast null to substrait typed null [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on code in PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#discussion_r2074660012
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -1590,6 +1590,21 @@ pub fn from_cast(
schema: &DFSchemaRef,
) -> Result<Expression> {
let Cast { expr, data_type } = cast;
+ // since substrait Null must be typed, so if we see a cast(null, dt), we make it a typed null
+ if let Expr::Literal(lit) = expr.as_ref() {
+ if lit.is_null() {
Review Comment:
> I don't really grok the `is_null` logic for compound types super well.
>
> I did test locally with
>
> ```rust
> let sv = ScalarValue::List(Arc::new(GenericListArray::new_null(
> Field::new_list_field(DataType::Float32, true).into(),
> 1,
> )));
> let target_type = DataType::List(
> Field::new_list_field(DataType::Int32, true).into()
> );
> let expr = Expr::Literal(sv)
> .cast_to(&target_type, &empty_schema)
> .unwrap();
> ```
>
> and was able to trigger the folding behaviour, which is undesirable... I think?
>
> @discord9 it might be good to capture that we _shouldn't_ fold this as a test.
All I ever wanted is to fold `SV::Null` since substrait need null to have a type, anyway will do in next PR since this one miss `try_cast` 's case and possibly missing some other cases
--
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: fold cast null to substrait typed null [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#issuecomment-2851983386
I restarted a failed CI run that looked to be due to some network error
--
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: fold cast null to substrait typed null [datafusion]
Posted by "discord9 (via GitHub)" <gi...@apache.org>.
discord9 commented on PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#issuecomment-2842039530
Realized `try_cast` have similiar problem, will try to fix it in next PR, the root issue is that a
`ScalarValue::Null` can't be properly translate to substrait's `Null(DataType)` since can't get data type from a SV::Null, will try refactor later in next PR for a better solution
--
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: fold cast null to substrait typed null [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#discussion_r2067003785
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -1590,6 +1590,21 @@ pub fn from_cast(
schema: &DFSchemaRef,
) -> Result<Expression> {
let Cast { expr, data_type } = cast;
+ // since substrait Null must be typed, so if we see a cast(null, dt), we make it a typed null
+ if let Expr::Literal(lit) = expr.as_ref() {
+ if lit.is_null() {
Review Comment:
The one thing I'm not sure about is the usage of `is_null()` here. I think the logic inside that works for non-compound types here:
```rust
pub fn is_null(&self) -> bool {
match self {
ScalarValue::Boolean(v) => v.is_none(),
ScalarValue::Null => true,
ScalarValue::Float16(v) => v.is_none(),
```
But for non-compound types I'm not sure if it does the right thing for the fold you're introducing:
```rust
ScalarValue::List(arr) => arr.len() == arr.null_count(),
ScalarValue::LargeList(arr) => arr.len() == arr.null_count(),
ScalarValue::FixedSizeList(arr) => arr.len() == arr.null_count(),
ScalarValue::Struct(arr) => arr.len() == arr.null_count(),
ScalarValue::Map(arr) => arr.len() == arr.null_count(),
```
because I don't think that it make sense to flatten empty compound types. If I understand how this would work, something like
```
Cast(List[], List<Int32>)
```
would get flattened into a null literal of type `List<i32>` which is subtly wrong, because it should be a empty `List<i32>`.
--
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: fold cast null to substrait typed null [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #15854:
URL: https://github.com/apache/datafusion/pull/15854#discussion_r2067003785
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -1590,6 +1590,21 @@ pub fn from_cast(
schema: &DFSchemaRef,
) -> Result<Expression> {
let Cast { expr, data_type } = cast;
+ // since substrait Null must be typed, so if we see a cast(null, dt), we make it a typed null
+ if let Expr::Literal(lit) = expr.as_ref() {
+ if lit.is_null() {
Review Comment:
The one thing I'm not sure about is the usage of `is_null()` here. I think the logic inside that works for non-compound types here:
```rust
pub fn is_null(&self) -> bool {
match self {
ScalarValue::Boolean(v) => v.is_none(),
ScalarValue::Null => true,
ScalarValue::Float16(v) => v.is_none(),
```
But for compound types I'm not sure if it does the right thing for the fold you're introducing:
```rust
ScalarValue::List(arr) => arr.len() == arr.null_count(),
ScalarValue::LargeList(arr) => arr.len() == arr.null_count(),
ScalarValue::FixedSizeList(arr) => arr.len() == arr.null_count(),
ScalarValue::Struct(arr) => arr.len() == arr.null_count(),
ScalarValue::Map(arr) => arr.len() == arr.null_count(),
```
because I don't think that it make sense to flatten empty compound types. If I understand how this would work, something like
```
Cast(List[], List<Int32>)
```
would get flattened into a null literal of type `List<i32>` which is subtly wrong, because it should be an empty `List<i32>`.
--
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