You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "gene-bordegaray (via GitHub)" <gi...@apache.org> on 2026/08/25 14:08:23 UTC
[PR] fix: preserve projection metadata during optimization [datafusion]
gene-bordegaray opened a new pull request, #24670:
URL: https://github.com/apache/datafusion/pull/24670
An identity projection can carry metadata even when its expressions match the input. Projection optimization currently turns:
`value {logical_type=event}` into `renamed {}`
Dropping the metadata changes the output schema and can cause schema mismatch errors. The fix removes identity projections only when their schemas match and copies the outer schema metadata when collapsing projection chains.
Tests: `cargo test -p datafusion-physical-plan projection::tests` and `cargo test -p datafusion-physical-optimizer projection_pushdown::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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3916681935
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1331,12 +1357,20 @@ pub fn update_join_filter(
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if outer.overrides_metadata()? {
+ return Ok(None);
+ }
Review Comment:
My agent flagged this as well.
@gene-bordegaray thisbcould be a good opportunity to run (locally, not worth committing) some branch coverage + mutation testing.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3896905406
##########
datafusion/proto-models/src/generated/prost.rs:
##########
@@ -1178,6 +1178,8 @@ pub struct CastNode {
>,
#[prost(bool, optional, tag = "4")]
pub nullable: ::core::option::Option<bool>,
+ #[prost(message, optional, tag = "5")]
+ pub target_field: ::core::option::Option<super::datafusion_common::Field>,
Review Comment:
This and other places in this file are adding `pub` fields also making this a breaking change.
##########
datafusion/expr/src/expr.rs:
##########
@@ -984,26 +983,76 @@ pub enum GetFieldAccess {
},
}
+/// Target of a cast expression.
+///
+/// A type-only target inherits metadata from the source expression. An explicit
+/// field supplies its own metadata, including an empty map that clears source
+/// metadata.
+#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
+pub enum CastTarget {
+ /// Cast to a data type while inheriting source metadata.
+ DataType(DataType),
+ /// Cast to an explicit field, including its metadata policy.
+ Field(FieldRef),
+}
+
+impl CastTarget {
+ /// Create a target that inherits source metadata.
+ pub fn type_only(data_type: DataType) -> Self {
+ Self::DataType(data_type)
+ }
+
+ /// Create an explicit field target.
+ pub fn explicit(field: FieldRef) -> Self {
+ Self::Field(field)
+ }
+
+ /// Return the target data type.
+ pub fn data_type(&self) -> &DataType {
+ match self {
+ Self::DataType(data_type) => data_type,
+ Self::Field(field) => field.data_type(),
+ }
+ }
+
+ /// Return the explicit target field, or `None` for a type-only target.
+ pub fn explicit_field(&self) -> Option<&FieldRef> {
+ match self {
+ Self::DataType(_) => None,
+ Self::Field(field) => Some(field),
+ }
+ }
+
+ /// Return explicit target metadata, or `None` when metadata is inherited.
+ pub fn metadata(&self) -> Option<&std::collections::HashMap<String, String>> {
+ self.explicit_field().map(|field| field.metadata())
+ }
+}
+
/// Cast expression
#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
pub struct Cast {
/// The expression being cast
pub expr: Box<Expr>,
- /// The `DataType` the expression will yield
- pub field: FieldRef,
+ /// The target type and metadata policy.
+ pub field: CastTarget,
Review Comment:
This is a breaking change of a public type, so if we go down this route this PR will not be eligible for a backport to 55. https://datafusion.apache.org/contributor-guide/release_management.html#backport-criteria
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3899537773
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -71,18 +71,23 @@ pub trait ExprSchemable {
-> Result<(DataType, bool)>;
}
-/// Derives the output field for a cast expression from the source field.
+/// Derives the output field for a cast expression from the source field, using
+/// explicit target metadata when supplied.
/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
fn cast_output_field(
source_field: &FieldRef,
- target_type: &DataType,
+ target: &CastTarget,
force_nullable: bool,
) -> Arc<Field> {
+ let metadata = target
+ .metadata()
+ .cloned()
+ .unwrap_or_else(|| source_field.metadata().clone());
let mut f = source_field
.as_ref()
.clone()
- .with_data_type(target_type.clone())
- .with_metadata(source_field.metadata().clone());
+ .with_data_type(target.data_type().clone())
+ .with_metadata(metadata);
Review Comment:
I will rebase now and the PR should clean up after #23169 merges
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5545748793
awesome thanks for all the detailed feedback on this 👍
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3865468014
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -71,18 +71,31 @@ pub trait ExprSchemable {
-> Result<(DataType, bool)>;
}
-/// Derives the output field for a cast expression from the source field.
+/// Derives the output field for a cast expression from the source and target
+/// fields. Type-only casts preserve source metadata, while an explicit target
+/// field supplies its own metadata.
+///
/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
fn cast_output_field(
source_field: &FieldRef,
- target_type: &DataType,
+ target_field: &FieldRef,
force_nullable: bool,
) -> Arc<Field> {
+ // `Cast::new` and `TryCast::new` use this field when only a target
+ // type is known. In that case, retain the source field's metadata.
+ let type_only_target = target_field.name().is_empty()
+ && target_field.is_nullable()
+ && target_field.metadata().is_empty();
+ let metadata = if type_only_target {
Review Comment:
Ya I thought this then AI actually ciaght this and I validated.
There can be the case where we are giving an target field with empty metadata and target type of a `FiexedSizeBinary`, then the source say has metadata that is marking something as a arrow UUID. In this case you would expect the result to be corrctly casted to `FixedSizeBinary` and empty metadata.
But what would happen is the target metadata is empty so then it would try to use the source. But the source metadata is saying to treated the `FixedSizeBinary` as a arrow UUID thus wouldnt cast correctly.
Now this check prevents that by checking if its a type only target.
There may be a clarner way to represent this though. I will return with thoughts
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3854382508
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1030,7 +1030,8 @@ pub fn remove_unnecessary_projections(
}
/// Compare the inputs and outputs of the projection. All expressions must be
-/// columns without alias, and projection does not change the order of fields.
+/// columns without alias, the projection must not change the order of fields,
+/// and its output schema must match its input schema, including metadata.
Review Comment:
Makes sense
--
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: preserve projection metadata during optimization [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5560766721
@gene-bordegaray Can you create a branch off `branch-55` and cherry pick the merge commit, create a PR targeting `branch-55`?
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3906054665
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
oh I saw that #23169 was approved. so we cannot have that check if that is going to be merged and in the patch as it will fail some of the tests it introduces. the commit on top solely fixes 1-3. I think that main qualm is that condition above, if we are keeping the sematics of casting the same for the patch (since we ae forced) for this to be fixed and pass CI after the merge then the check cannot be as simple as above.
Maybe we could kee my check and add a todo, ensuring that these semantics are meant to be reviewed for a longer term solution? Let me knwo your thoughts
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5412986071
> Do you think it's related to this change?
Found issues, this is a bit more involved than I was hoping. Will the variants with the fix
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3919761930
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1331,12 +1357,20 @@ pub fn update_join_filter(
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if outer.overrides_metadata()? {
+ return Ok(None);
+ }
Review Comment:
wow this is a cool concept, I haven't heard of this method. Doing right now, I will report back here
Here is the the case I added with an agent to do many mutations:
```text
Projection output field: i
metadata={"event_field":"true"}
Original child field: i
metadata={}
```
So the projection adds metadata and we check that we override metadata.
Then replaces the child with one that has the same metadata:
```text
Projection output field: i
metadata={"event_field":"true"}
Replacement child field: i
metadata={"event_field":"true"}
```
Now the projection is no longer adding metadata and we check that `reclaculte_children` switches `override_metadata` to false.
When the implementation to copies the old value:
```rust
let overrides_metadata = self.overrides_metadata;
```
the test fails. But this wasn't covered before
--
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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3907966331
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
> This is my first participation in a patch release so some guidance would be great. Thank you again 🙇
You're doing great! The main thing is trying to minimize the amount of code and behavior change that ships with a patch. And in general keeping PRs decoupled (e.g. splitting a bit PR into two smaller ones so that at least one of them is unblocked) is good. Those were my suggestions. Sorry if they didn't make sense here.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5561125149
> @gene-bordegaray Can you create a branch off `branch-55` and cherry pick the merge commit, create a PR targeting `branch-55`?
yes, done: #24992
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3927657601
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -83,6 +83,9 @@ pub struct ProjectionExec {
metrics: ExecutionPlanMetricsSet,
/// Cache holding plan properties like equivalences, output partitioning etc.
cache: Arc<PlanProperties>,
+ /// Whether the output metadata differs from the metadata derived from the
+ /// projection expressions and input schema.
+ overrides_metadata: bool,
Review Comment:
I believe this actually wont happen because in our `replace_children` implementation:
- on a `Keep` we clone this value with is ok because we are saying the child will have the same `PlanProterties` as the old child
- on a `Recompute` we do pass the new input schema to `compute_overrides_metadata` and recompute the flag.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3919092679
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1331,12 +1357,20 @@ pub fn update_join_filter(
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if outer.overrides_metadata()? {
+ return Ok(None);
+ }
Review Comment:
If you run some let me know how it goes. I'd be interested to know if it was useful, and if so if any bugs were found, etc.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5438238594
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-expr v55.0.0 (current)
Built [ 29.380s] (current)
Parsing datafusion-expr v55.0.0 (current)
Parsed [ 0.078s] (current)
Building datafusion-expr v55.0.0 (baseline)
Built [ 29.259s] (baseline)
Parsing datafusion-expr v55.0.0 (baseline)
Parsed [ 0.075s] (baseline)
Checking datafusion-expr v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 1.226s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 61.357s] datafusion-expr
Building datafusion-functions v55.0.0 (current)
Built [ 31.745s] (current)
Parsing datafusion-functions v55.0.0 (current)
Parsed [ 0.083s] (current)
Building datafusion-functions v55.0.0 (baseline)
Built [ 31.265s] (baseline)
Parsing datafusion-functions v55.0.0 (baseline)
Parsed [ 0.086s] (baseline)
Checking datafusion-functions v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.380s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 64.903s] datafusion-functions
Building datafusion-physical-expr v55.0.0 (current)
Built [ 29.447s] (current)
Parsing datafusion-physical-expr v55.0.0 (current)
Parsed [ 0.048s] (current)
Building datafusion-physical-expr v55.0.0 (baseline)
Built [ 29.398s] (baseline)
Parsing datafusion-physical-expr v55.0.0 (baseline)
Parsed [ 0.049s] (baseline)
Checking datafusion-physical-expr v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.336s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 60.099s] datafusion-physical-expr
Building datafusion-physical-plan v55.0.0 (current)
Built [ 38.395s] (current)
Parsing datafusion-physical-plan v55.0.0 (current)
Parsed [ 0.149s] (current)
Building datafusion-physical-plan v55.0.0 (baseline)
Built [ 38.619s] (baseline)
Parsing datafusion-physical-plan v55.0.0 (baseline)
Parsed [ 0.150s] (baseline)
Checking datafusion-physical-plan v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.669s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 79.320s] datafusion-physical-plan
Building datafusion-proto v55.0.0 (current)
Built [ 54.272s] (current)
Parsing datafusion-proto v55.0.0 (current)
Parsed [ 0.017s] (current)
Building datafusion-proto v55.0.0 (baseline)
Built [ 54.351s] (baseline)
Parsing datafusion-proto v55.0.0 (baseline)
Parsed [ 0.018s] (baseline)
Checking datafusion-proto v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.135s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 109.940s] datafusion-proto
Building datafusion-proto-models v55.0.0 (current)
Built [ 25.402s] (current)
Parsing datafusion-proto-models v55.0.0 (current)
Parsed [ 0.129s] (current)
Building datafusion-proto-models v55.0.0 (baseline)
Built [ 26.018s] (baseline)
Parsing datafusion-proto-models v55.0.0 (baseline)
Parsed [ 0.132s] (baseline)
Checking datafusion-proto-models v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 1.791s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---
Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field PhysicalCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1859
field PhysicalCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1859
field CastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1182
field CastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1182
field PhysicalTryCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1850
field PhysicalTryCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1850
field TryCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1198
field TryCastNode.target_field in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1198
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 54.868s] datafusion-proto-models
Building datafusion-pruning v55.0.0 (current)
Built [ 40.840s] (current)
Parsing datafusion-pruning v55.0.0 (current)
Parsed [ 0.012s] (current)
Building datafusion-pruning v55.0.0 (baseline)
Built [ 41.190s] (baseline)
Parsing datafusion-pruning v55.0.0 (baseline)
Parsed [ 0.013s] (baseline)
Checking datafusion-pruning v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.079s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 83.079s] datafusion-pruning
Building datafusion-sql v55.0.0 (current)
Built [ 44.110s] (current)
Parsing datafusion-sql v55.0.0 (current)
Parsed [ 0.031s] (current)
Building datafusion-sql v55.0.0 (baseline)
Built [ 44.269s] (baseline)
Parsing datafusion-sql v55.0.0 (baseline)
Parsed [ 0.032s] (baseline)
Checking datafusion-sql v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.259s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 90.099s] datafusion-sql
Building datafusion-substrait v55.0.0 (current)
Built [ 315.979s] (current)
Parsing datafusion-substrait v55.0.0 (current)
Parsed [ 0.018s] (current)
Building datafusion-substrait v55.0.0 (baseline)
Built [ 319.802s] (baseline)
Parsing datafusion-substrait v55.0.0 (baseline)
Parsed [ 0.017s] (baseline)
Checking datafusion-substrait v55.0.0 -> v55.0.0 (no change; assume patch)
Checked [ 0.218s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 637.962s] datafusion-substrait
```
</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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3903228823
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
Yes, it is for a different bug but when i rooginally made the minimal fix (my top commit now on this brnach) it fails some sqllogictests because of #23169 not being merged. Specifically:
```sql
SELECT CAST(raw AS UUID), arrow_metadata(CAST(raw AS UUID), 'ARROW:extension:name');
```
Expected on main:
```text
00010203040506070809000102030506 arrow.uuid
```
But with the projection fix alone the projection starts to actually preserver logical schema then ignores the UUID target metadata, so the result:
```diff
- arrow.uuid
+ NULL
```
#24831 handles this with this check:
```rust
let metadata = if target_field.metadata().is_empty() {
source_field.metadata().clone()
} else {
target_field.metadata().clone()
};
```
as prposed in your brnach #24831 but it has the issue that I talk about [here](https://github.com/apache/datafusion/pull/24670#discussion_r3865163140) which is hwy I opted into stacking on #23169 to handle that case while not allowing another edge case to creep in.
So this is getting a bit tricky to handle. That particular test is from another PR, but solving just the bug at at the surface level unveils more underlying issues with casting and metadata that this relies on. I would think that getting this in the minor patch with #23169 would be the good short term solution, Then I read [your comment](https://github.com/apache/datafusion/pull/23169#issuecomment-5489686098) and I think this could be a viable breaking change after some more discussion regarding how we want these semantics to behave.
Thanks for taking time to investigate all this @adriangb 🙇
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3899536784
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -71,18 +71,23 @@ pub trait ExprSchemable {
-> Result<(DataType, bool)>;
}
-/// Derives the output field for a cast expression from the source field.
+/// Derives the output field for a cast expression from the source field, using
+/// explicit target metadata when supplied.
/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
fn cast_output_field(
source_field: &FieldRef,
- target_type: &DataType,
+ target: &CastTarget,
force_nullable: bool,
) -> Arc<Field> {
+ let metadata = target
+ .metadata()
+ .cloned()
+ .unwrap_or_else(|| source_field.metadata().clone());
let mut f = source_field
.as_ref()
.clone()
- .with_data_type(target_type.clone())
- .with_metadata(source_field.metadata().clone());
+ .with_data_type(target.data_type().clone())
+ .with_metadata(metadata);
Review Comment:
yes sorry about the confusion I should've temporarily made this a draft, the diff for this PR is much smaller. Allthat the `CastTarget` and protobuf changes are mixed into this PR because I had stacked it on #24725 but they shouldn’t be reviewed as part of the projection optimizer fix. I’m going to rebase this onto #23169 after some discussion to go with that approach for the minor relese.
as far as this particular comment. I originally thought this too and @gabotechs also asked about [this](https://github.com/apache/datafusion/pull/24670#discussion_r3865163140), its a subtle one. Let me know if that clarifies 👍
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3907981873
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
ok cool, thanks! I think the current state after the prerequisite prs got merged is more along the lines of what we are lookiing for in this
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5412618322
> Do you think it's related to this change?
looking into
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5544510066
@timsaucer caught a guard on an outer metadata guard that couldn't be reached, merged those in. Thank you 👍
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3899771450
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
this test is the one that would fail in #24831 @adriangb , it was added in #23169
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3918064152
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
gonna resolve this guy now 👍
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3919761930
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1331,12 +1357,20 @@ pub fn update_join_filter(
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if outer.overrides_metadata()? {
+ return Ok(None);
+ }
Review Comment:
wow this is a cool concept, I haven't heard of this method. Doing right now, I will report back here
--
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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5545035884
lgtm from my end. thanks @timsaucer and @gene-bordegaray for driving this
--
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: preserve projection metadata during optimization [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5544932949
@gabotechs @adriangb Any final comments or thoughts on this PR before we merge?
--
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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3898881697
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -71,18 +71,23 @@ pub trait ExprSchemable {
-> Result<(DataType, bool)>;
}
-/// Derives the output field for a cast expression from the source field.
+/// Derives the output field for a cast expression from the source field, using
+/// explicit target metadata when supplied.
/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
fn cast_output_field(
source_field: &FieldRef,
- target_type: &DataType,
+ target: &CastTarget,
force_nullable: bool,
) -> Arc<Field> {
+ let metadata = target
+ .metadata()
+ .cloned()
+ .unwrap_or_else(|| source_field.metadata().clone());
let mut f = source_field
.as_ref()
.clone()
- .with_data_type(target_type.clone())
- .with_metadata(source_field.metadata().clone());
+ .with_data_type(target.data_type().clone())
+ .with_metadata(metadata);
Review Comment:
This is the only change I needed on top of `projection.rs` to make the UUID test pass (counterfactual confirmed: reverting just this reproduces the CI failure).
Note your callsite changes below — `cast_output_field(&src, field, false)` / `(&src, field, true)` — work **unchanged** with this, since `Cast.field` would go back to being a `FieldRef`. So this suggestion plus dropping `CastTarget` is the whole edit; the other ~20 files in the stack come out with it.
```suggestion
/// Derives the output field for a cast expression from the source field.
///
/// The cast target's metadata is authoritative when it carries any; otherwise the
/// source's metadata is inherited. This mirrors the physical `CastExpr`, whose
/// target field is already authoritative when it is not the synthesized type-only
/// field.
///
/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
fn cast_output_field(
source_field: &FieldRef,
target_field: &FieldRef,
force_nullable: bool,
) -> Arc<Field> {
let metadata = if target_field.metadata().is_empty() {
source_field.metadata().clone()
} else {
target_field.metadata().clone()
};
let mut f = source_field
.as_ref()
.clone()
.with_data_type(target_field.data_type().clone())
.with_metadata(metadata);
```
--
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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3905432158
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
> it has the issue that I talk about https://github.com/apache/datafusion/pull/24670#discussion_r3865163140 which is hwy I opted into stacking on https://github.com/apache/datafusion/pull/23169 to handle that case while not allowing another edge case to creep in
The discussion you linked is suggesting that we go with:
> Does target_field have metadata? if yes, then use that metadata, if not, then use the source_field metadata.
You propose that breaks with: `arrow_cast(uuid_col, 'FixedSizeBinary')` because the result is `FixedSizeBinary` but with the UUID metadata (invalid).
Under the stricter proposal in https://github.com/apache/datafusion/pull/23169#issuecomment-5489686098 this would be resolved: we'd ignore the source field metadata.
I think the larger question is if we back port the behavior change to `55`.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3916480513
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1331,12 +1357,20 @@ pub fn update_join_filter(
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if outer.overrides_metadata()? {
+ return Ok(None);
+ }
Review Comment:
yes I manually confirmed this and this is true. When collapsing we are reconstructing the combined projection with the outer schema so this guy is redundant while the inner projection guard isn't.
It was added because I overlook this 😄
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3865359658
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1326,17 +1340,44 @@ pub fn update_join_filter(
})
}
+/// Returns whether a projection defines metadata that its expressions and input
+/// schema cannot reproduce.
+///
+/// Such a projection is an execution boundary: a parent expression such as
+/// `arrow_metadata` can observe its output field metadata.
+fn projection_overrides_metadata(projection: &ProjectionExec) -> Result<bool> {
+ let derived_schema = projection
+ .projector
+ .projection()
+ .project_schema(projection.input().schema().as_ref())?;
+ let output_schema = projection.schema();
+ Ok(derived_schema.metadata() != output_schema.metadata()
+ || derived_schema
+ .fields()
+ .iter()
+ .zip(output_schema.fields())
+ .any(|(derived, output)| derived.metadata() != output.metadata()))
+}
+
/// Collapse a chain of consecutive [`ProjectionExec`]s into one. Returns
/// `None` if nothing could be merged.
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if projection_overrides_metadata(outer)? {
+ return Ok(None);
+ }
+
let mut current_exprs: Vec<ProjectionExpr> = outer.expr().to_vec();
let mut current_input: Arc<dyn ExecutionPlan> = Arc::clone(outer.input());
let mut column_ref_map: HashMap<Column, usize> = HashMap::new();
let mut collapsed_any = false;
'outer: while let Some(inner_proj) = current_input.downcast_ref::<ProjectionExec>() {
+ if projection_overrides_metadata(inner_proj)? {
+ break;
+ }
+
// Collect the column references usage in the outer projection.
Review Comment:
This is sometimes collapsable because take the example I put in the PR description. You might have this:
```text
ProjectionExec: arrow_metadata(i, 'event_field') AS metadata
ProjectionExec: i@0 AS i
output metadata = {"event_field": "true"}
DataSourceExec: i
metadata = {}
```
The outer query does not have override metadata but the inner does. The correct result is `true`.
The previous projection logic would collapse these, but the outer projection expr reads the metadata. So it would make:
```text
ProjectionExec: arrow_metadata(i, 'event_field') AS metadata
DataSourceExec: i
metadata = {}
```
Giving use result as `NULL` now.
This is kinda ocnservative as we could probably just chekc if everything in the outer projection is just referencing columns but I am just trying to get to correctness first. Then could do the optimixation. What you think?
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3865359658
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1326,17 +1340,44 @@ pub fn update_join_filter(
})
}
+/// Returns whether a projection defines metadata that its expressions and input
+/// schema cannot reproduce.
+///
+/// Such a projection is an execution boundary: a parent expression such as
+/// `arrow_metadata` can observe its output field metadata.
+fn projection_overrides_metadata(projection: &ProjectionExec) -> Result<bool> {
+ let derived_schema = projection
+ .projector
+ .projection()
+ .project_schema(projection.input().schema().as_ref())?;
+ let output_schema = projection.schema();
+ Ok(derived_schema.metadata() != output_schema.metadata()
+ || derived_schema
+ .fields()
+ .iter()
+ .zip(output_schema.fields())
+ .any(|(derived, output)| derived.metadata() != output.metadata()))
+}
+
/// Collapse a chain of consecutive [`ProjectionExec`]s into one. Returns
/// `None` if nothing could be merged.
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if projection_overrides_metadata(outer)? {
+ return Ok(None);
+ }
+
let mut current_exprs: Vec<ProjectionExpr> = outer.expr().to_vec();
let mut current_input: Arc<dyn ExecutionPlan> = Arc::clone(outer.input());
let mut column_ref_map: HashMap<Column, usize> = HashMap::new();
let mut collapsed_any = false;
'outer: while let Some(inner_proj) = current_input.downcast_ref::<ProjectionExec>() {
+ if projection_overrides_metadata(inner_proj)? {
+ break;
+ }
+
// Collect the column references usage in the outer projection.
Review Comment:
This is sometimes collapsable because take the example I put in the PR description. You might have this:
```text
ProjectionExec: arrow_metadata(i, 'event_field') AS metadata
ProjectionExec: i@0 AS i
output metadata = {"event_field": "true"}
DataSourceExec: i
metadata = {}
```
The outer query does not have override metadata but the inner does. The correct result is `true`.
The previous projection logic would collapse these, but the outer projection expr reads the metadata. This wa the CI failure. So it would make:
```text
ProjectionExec: arrow_metadata(i, 'event_field') AS metadata
DataSourceExec: i
metadata = {}
```
Giving use result as `NULL` now.
This is kinda ocnservative as we could probably just chekc if everything in the outer projection is just referencing columns but I am just trying to get to correctness first. Then could do the optimixation. What you think?
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5419154156
this is also a correctenss issue / regression in 55 so I can note this in the minor version bump
--
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: preserve projection metadata during optimization [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5419455162
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24670?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 `86.86131%` with `18 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.45%. Comparing base ([`63f5b55`](https://app.codecov.io/gh/apache/datafusion/commit/63f5b55fb071707d2d78e18805df1ab365e1fd93?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f64100d`](https://app.codecov.io/gh/apache/datafusion/commit/f64100dde6d8f2f698bd5609d70e94d5081a7dde?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24670?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/physical-plan/src/projection.rs](https://app.codecov.io/gh/apache/datafusion/pull/24670?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fprojection.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9wcm9qZWN0aW9uLnJz) | 84.15% | [4 Missing and 12 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24670?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/expr/src/expr\_schema.rs](https://app.codecov.io/gh/apache/datafusion/pull/24670?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Fexpr_schema.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9leHByX3NjaGVtYS5ycw==) | 94.44% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24670?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 #24670 +/- ##
==========================================
+ Coverage 81.44% 81.45% +0.01%
==========================================
Files 1118 1118
Lines 399550 399677 +127
Branches 399550 399677 +127
==========================================
+ Hits 325398 325554 +156
+ Misses 55154 55107 -47
- Partials 18998 19016 +18
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24670?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: preserve projection metadata during optimization [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3865163140
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -71,18 +71,31 @@ pub trait ExprSchemable {
-> Result<(DataType, bool)>;
}
-/// Derives the output field for a cast expression from the source field.
+/// Derives the output field for a cast expression from the source and target
+/// fields. Type-only casts preserve source metadata, while an explicit target
+/// field supplies its own metadata.
+///
/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
fn cast_output_field(
source_field: &FieldRef,
- target_type: &DataType,
+ target_field: &FieldRef,
force_nullable: bool,
) -> Arc<Field> {
+ // `Cast::new` and `TryCast::new` use this field when only a target
+ // type is known. In that case, retain the source field's metadata.
+ let type_only_target = target_field.name().is_empty()
+ && target_field.is_nullable()
+ && target_field.metadata().is_empty();
+ let metadata = if type_only_target {
Review Comment:
Why is it not sufficient to do something simpler like:
Does `target_field` have metadata? if yes, then use that metadata, if not, then use the `source_field` metadata.
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1326,17 +1340,44 @@ pub fn update_join_filter(
})
}
+/// Returns whether a projection defines metadata that its expressions and input
+/// schema cannot reproduce.
+///
+/// Such a projection is an execution boundary: a parent expression such as
+/// `arrow_metadata` can observe its output field metadata.
+fn projection_overrides_metadata(projection: &ProjectionExec) -> Result<bool> {
Review Comment:
Nit: a `ProjectionExec::overrides_metadata` method rather than a standalone function sounds very slightly more elegant.
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1326,17 +1340,44 @@ pub fn update_join_filter(
})
}
+/// Returns whether a projection defines metadata that its expressions and input
+/// schema cannot reproduce.
+///
+/// Such a projection is an execution boundary: a parent expression such as
+/// `arrow_metadata` can observe its output field metadata.
+fn projection_overrides_metadata(projection: &ProjectionExec) -> Result<bool> {
+ let derived_schema = projection
+ .projector
+ .projection()
+ .project_schema(projection.input().schema().as_ref())?;
+ let output_schema = projection.schema();
+ Ok(derived_schema.metadata() != output_schema.metadata()
+ || derived_schema
+ .fields()
+ .iter()
+ .zip(output_schema.fields())
+ .any(|(derived, output)| derived.metadata() != output.metadata()))
+}
+
/// Collapse a chain of consecutive [`ProjectionExec`]s into one. Returns
/// `None` if nothing could be merged.
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if projection_overrides_metadata(outer)? {
+ return Ok(None);
+ }
+
let mut current_exprs: Vec<ProjectionExpr> = outer.expr().to_vec();
let mut current_input: Arc<dyn ExecutionPlan> = Arc::clone(outer.input());
let mut column_ref_map: HashMap<Column, usize> = HashMap::new();
let mut collapsed_any = false;
'outer: while let Some(inner_proj) = current_input.downcast_ref::<ProjectionExec>() {
+ if projection_overrides_metadata(inner_proj)? {
+ break;
+ }
+
// Collect the column references usage in the outer projection.
Review Comment:
Imagine this situation:
```
ProjectionExec: <- does not override metadata
ProjectionExec: <- overrides metadata
```
This is collapsible right? but the current code will omit collapsing it.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5437480771
ok I pushed a change that introduces an enum to differentiation between data type and explicit field casts. It is a larger and public api change but it is what I see as properly tracking this information, not an ad hoc check
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3854265181
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1030,7 +1030,8 @@ pub fn remove_unnecessary_projections(
}
/// Compare the inputs and outputs of the projection. All expressions must be
-/// columns without alias, and projection does not change the order of fields.
+/// columns without alias, the projection must not change the order of fields,
+/// and its output schema must match its input schema, including metadata.
Review Comment:
This was existing behavior but happens because if we removed a projection that flipped order then we might get wrong results if the ordering was part of the query like this:
Input plan
```text
DataSourceExec
schema: [value:Int32, value:Int32]
row: [10, 20]
```
The projection then swaps them:
```text
ProjectionExec: expr=[value@1, value@0]
schema: [value:Int32, value:Int32]
row: [20, 10]
DataSourceExec
schema: [value:Int32, value:Int32]
row: [10, 20]
```
The projection stays because the indices do not match their output positions with that check. So we get the right result: `[20, 10]`
If we removed it then the names and schemas match, so the optimizer could remove the projection:
```text
DataSourceExec
row: [10, 20]
```
So [20, 10] order gets lost.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5425952348
created codex / serialization follow up here: #24695
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3927677162
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -270,6 +278,33 @@ impl ProjectionExec {
))
}
+ /// Returns whether `projector`'s output metadata differs from the metadata
+ /// derived from its expressions and `input_schema`.
+ fn compute_overrides_metadata(
+ projector: &Projector,
+ input_schema: &Schema,
+ ) -> Result<bool> {
+ let output_schema = projector.output_schema();
+ if input_schema.metadata() != output_schema.metadata() {
+ return Ok(true);
+ }
+ for (projection, output_field) in
+ projector.projection().iter().zip(output_schema.fields())
+ {
+ let derived_field = projection.expr.return_field(input_schema)?;
+ if derived_field.metadata() != output_field.metadata() {
+ return Ok(true);
+ }
+ }
+ Ok(false)
Review Comment:
yes we could. The only thing is `project_schema` does a ton of extra stuff we don't really need. So with the past wide schema perf concerns I think just deriving exactly what we need is gonna give us better perf here
--
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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3901124464
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
That's a new test from https://github.com/apache/datafusion/pull/23169 right? Isn't that a test for a _different_ bug than https://github.com/apache/datafusion/issues/24721, i.e. we don't need to couple solving them?
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5514612020
also so I understand, what are the testing concerns that https://github.com/gene-bordegaray/datafusion/pull/7 addresses? I dont see these tests exposing any of the behavior changes in this PR, more testing #23169 behavior.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3915183167
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -270,6 +270,22 @@ impl ProjectionExec {
))
}
+ /// Returns whether this projection's output metadata differs from the
+ /// metadata derived from its expressions and input schema.
+ fn overrides_metadata(&self) -> Result<bool> {
+ let derived_schema = self
+ .projector
+ .projection()
+ .project_schema(self.input.schema().as_ref())?;
+ let output_schema = self.schema();
+ Ok(derived_schema.metadata() != output_schema.metadata()
+ || derived_schema
+ .fields()
+ .iter()
+ .zip(output_schema.fields())
+ .any(|(derived, output)| derived.metadata() != output.metadata()))
+ }
Review Comment:
yes makes sense
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3906054665
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
oh I saw that #23169 was approved. so we cannot have that check if that is going to be merged and in the patch as it will fail some of the tests it introduces. the commit on top solely fixes 1-3. I think that main qualm is that condition above, if we are keeping the sematics of casting the same for the patch (since we ae forced) for this to be fixed and pass CI after the merge then the check cannot be as simple as above.
Maybe we could kee my check and add a todo, ensuring that these semantics are meant to be reviewed for a longer term solution? Let me knwo your thoughts
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3905990481
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
yes we could and have CI pass, but there will be the undelying bug that the other PR is talking about and the check prposed elides. If we are aware of that and ok with it then more than happy to just have the top commit and use the check:
```rust
let metadata = if target_field.metadata().is_empty() {
source_field.metadata().clone()
} else {
target_field.metadata().clone()
};
```
This is my first participation in a patch release so some guidance would be great. Thank you again 🙇
EDIT: #23169 properly handles this so if we backpoirt this with it should fix all cases for now until we discuss long term semantics
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3854376229
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1041,6 +1042,7 @@ fn is_projection_removable(projection: &ProjectionExec) -> bool {
};
col.name() == proj_expr.alias && col.index() == idx
}) && exprs.len() == projection.input().schema().fields().len()
+ && projection.schema() == projection.input().schema()
Review Comment:
Pretty much the order of columns. I bet that's why the current checks are like they are right now.
I think it's fine though, if this becomes too restrictive it will start popping up in tests
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5412518611
🤔 there seems to be a CI failure:
```
1. query result mismatch:
[SQL] SELECT
CAST(raw AS UUID),
arrow_metadata(CAST(raw AS UUID), 'ARROW:extension:name')
FROM (
VALUES (
arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
)
) AS uuids(raw);
[Diff] (-expected|+actual)
- 00010203040506070809000102030506 arrow.uuid
+ 00010203040506070809000102030506 NULL
at /home/runner/work/datafusion/datafusion/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:36
```
Do you think it's related to this change?
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5419118429
@gabotechs ok I figured out what was going on and documented it in the PR description. There is also another bug in the codec / serialization where we need to serialize metadata. I am not solving that in this PR to keep scoped / tracked. I will crete issue for this tmrw or you can if you would like 👍
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3854285103
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1041,6 +1042,7 @@ fn is_projection_removable(projection: &ProjectionExec) -> bool {
};
col.name() == proj_expr.alias && col.index() == idx
}) && exprs.len() == projection.input().schema().fields().len()
+ && projection.schema() == projection.input().schema()
Review Comment:
We need to check full schema because even if the schema metadata is equal things like the field metadata might not be thus we nee to check this as well.
I don't see anything in the schema which would be overestricting this. I may be missing something though
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3899537773
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -71,18 +71,23 @@ pub trait ExprSchemable {
-> Result<(DataType, bool)>;
}
-/// Derives the output field for a cast expression from the source field.
+/// Derives the output field for a cast expression from the source field, using
+/// explicit target metadata when supplied.
/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
fn cast_output_field(
source_field: &FieldRef,
- target_type: &DataType,
+ target: &CastTarget,
force_nullable: bool,
) -> Arc<Field> {
+ let metadata = target
+ .metadata()
+ .cloned()
+ .unwrap_or_else(|| source_field.metadata().clone());
let mut f = source_field
.as_ref()
.clone()
- .with_data_type(target_type.clone())
- .with_metadata(source_field.metadata().clone());
+ .with_data_type(target.data_type().clone())
+ .with_metadata(metadata);
Review Comment:
I will rebase now and the PR should clean up
--
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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3901124464
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
That's a new test from https://github.com/apache/datafusion/pull/23169 right? It's new behavior, not a regression test for a bug.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3905990481
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
yes we could and have CI pass, but there will be the undelying bug that the other PR is talking about and the check prposed elides. If we are aware of that and ok with it then more than happy to just have the top commit and use the check:
```rust
let metadata = if target_field.metadata().is_empty() {
source_field.metadata().clone()
} else {
target_field.metadata().clone()
};
```
This is my first participation in a patch release so some guidance would be great. Thank you again 🙇
--
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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3905935164
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
Since this PR is fixing 4 issues, can we merge a fix for 1-3 without 4?
--
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: preserve projection metadata during optimization [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3913901198
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -270,6 +270,22 @@ impl ProjectionExec {
))
}
+ /// Returns whether this projection's output metadata differs from the
+ /// metadata derived from its expressions and input schema.
+ fn overrides_metadata(&self) -> Result<bool> {
+ let derived_schema = self
+ .projector
+ .projection()
+ .project_schema(self.input.schema().as_ref())?;
+ let output_schema = self.schema();
+ Ok(derived_schema.metadata() != output_schema.metadata()
+ || derived_schema
+ .fields()
+ .iter()
+ .zip(output_schema.fields())
+ .any(|(derived, output)| derived.metadata() != output.metadata()))
+ }
Review Comment:
From an agent, but it looks like a good idea to me. Specifically I've seen things like this bite when you have very wide schemas.
- Perf: overrides_metadata() recomputes full schema derivation every pass.
`project_schema` walks every expr 3× (`return_field`, `data_type`, `nullable`) and allocates a Schema. Called via transform_down for every ProjectionExec node, plus once per inner projection in collapse loop, plus recursively at projection.rs:1430. Wide projections pay repeatedly.
Cache bool once in try_from_projector as a struct field. try_new path always false; only try_new_with_schema_metadata needs the compare.
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1331,12 +1357,20 @@ pub fn update_join_filter(
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if outer.overrides_metadata()? {
+ return Ok(None);
+ }
Review Comment:
From an agent's review. I didn't track this one through myself so feel free to push back if you disagree.
- Outer guard in try_collapse_projection_chain looks unnecessary.
Line 1425 already forces outer.schema() metadata onto the unified projection. The inner guard (line 1370) is the one carrying the correctness weight — substituting through an inner projection loses metadata the outer expr observes. Dropping the outer bail keeps collapse working for extension-type casts. Intentional extra caution, or leftover from an earlier revision?
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1014,6 +1030,10 @@ pub fn remove_unnecessary_projections(
plan: Arc<dyn ExecutionPlan>,
) -> Result<Transformed<Arc<dyn ExecutionPlan>>> {
let maybe_modified = if let Some(projection) = plan.downcast_ref::<ProjectionExec>() {
+ // Removing a projection with observable metadata can change query results.
+ if projection.overrides_metadata()? {
+ return Ok(Transformed::no(plan));
+ }
Review Comment:
After some back and forth with an agent, this now seems like a reasonable issue about this check:
This is a precondition, so it exits before `try_swapping_with_projection` — it blocks relocation as well as removal. The 20 `try_swapping_with_projection` impls split into two groups:
- **7 relocate** the projection via `make_with_child` — which this PR already made metadata-safe
- **3 absorb** it (`source.rs:525` covering all `DataSource` impls, `streaming.rs:337`) and **1 embeds** it (`try_embed_projection`, covering the `FilterExec` fallback and all 5 joins) — these re-derive their schema and do drop the metadata
The precondition can't distinguish them, so it pays for the lossy 4 by disabling the safe 7. The catch is who that lands on: `overrides_metadata()` only fires for projections built through `try_new_with_schema_metadata`, which after #23169 means embedder-constructed plans, not SQL. So the users who need this fix are exactly the ones who lose the pushdown.
Concretely:
```
ProjectionExec: a@0 AS a declared output: a -> {"unit": "ms"}
FilterExec: a@0 > 5
DataSourceExec: a, b, c
```
| | metadata | filter reads |
| ---------------- | -------- | ------------ |
| pre-PR | **lost** | 1 col |
| this PR | kept | **3 cols** |
| suggestion below | kept | 1 col |
Would it work to make it a postcondition instead — check the result rather than deciding up front?
```rust
projection
.input()
.try_swapping_with_projection(projection)?
.filter(|swapped| swapped.schema() == projection.schema())
```
Deleting the precondition should be free: the removal path is already covered by the `projection.schema() == projection.input().schema()` you added to `is_projection_removable`, so the precondition was only ever guarding the swap. This also means a future `try_swapping_with_projection` impl is covered without anyone remembering to add a guard — the failure mode becomes a lost optimization rather than lost metadata. And it adds no public API, which I think keeps this patch-eligible.
I tried it against `d112760`:
| Check | Result |
| ----------------------------------- | ----------------------------------------------- |
| Filter relocation (safe path) | swap happens, metadata preserved |
| `try_embed_projection` (lossy path) | swap rejected, projection kept, metadata intact |
| Full sqllogictest suite, 510 files | zero false rejections |
| slt failures vs. unmodified PR head | identical (same 35 files) |
| `projection::tests` | 24/24 pass, including all 4 you added |
The rejection trace on the lossy path shows the metadata that would have been dropped:
```
SWAP_REJECTED child=FilterExec
proj_schema= [Field { name: "a", metadata: {"unit": "ms"} }]
swapped_schema= [Field { name: "a" }]
```
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3853980928
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1030,7 +1030,8 @@ pub fn remove_unnecessary_projections(
}
/// Compare the inputs and outputs of the projection. All expressions must be
-/// columns without alias, and projection does not change the order of fields.
+/// columns without alias, the projection must not change the order of fields,
+/// and its output schema must match its input schema, including metadata.
Review Comment:
🤔 Why can't it change the order of fields? As long as the name is the same one, that's what matters AFAIK.
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1041,6 +1042,7 @@ fn is_projection_removable(projection: &ProjectionExec) -> bool {
};
col.name() == proj_expr.alias && col.index() == idx
}) && exprs.len() == projection.input().schema().fields().len()
+ && projection.schema() == projection.input().schema()
Review Comment:
This might be putting more restrictions than just metadata equality. It might be fine, but if we want to play it safe it could be better to just do `&& projection.schema().metadata() == projection.input().schema().metadata()`
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3865408726
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1326,17 +1340,44 @@ pub fn update_join_filter(
})
}
+/// Returns whether a projection defines metadata that its expressions and input
+/// schema cannot reproduce.
+///
+/// Such a projection is an execution boundary: a parent expression such as
+/// `arrow_metadata` can observe its output field metadata.
+fn projection_overrides_metadata(projection: &ProjectionExec) -> Result<bool> {
+ let derived_schema = projection
+ .projector
+ .projection()
+ .project_schema(projection.input().schema().as_ref())?;
+ let output_schema = projection.schema();
+ Ok(derived_schema.metadata() != output_schema.metadata()
+ || derived_schema
+ .fields()
+ .iter()
+ .zip(output_schema.fields())
+ .any(|(derived, output)| derived.metadata() != output.metadata()))
+}
+
/// Collapse a chain of consecutive [`ProjectionExec`]s into one. Returns
/// `None` if nothing could be merged.
fn try_collapse_projection_chain(
outer: &ProjectionExec,
) -> Result<Option<Arc<dyn ExecutionPlan>>> {
+ if projection_overrides_metadata(outer)? {
+ return Ok(None);
+ }
+
let mut current_exprs: Vec<ProjectionExpr> = outer.expr().to_vec();
let mut current_input: Arc<dyn ExecutionPlan> = Arc::clone(outer.input());
let mut column_ref_map: HashMap<Column, usize> = HashMap::new();
let mut collapsed_any = false;
'outer: while let Some(inner_proj) = current_input.downcast_ref::<ProjectionExec>() {
+ if projection_overrides_metadata(inner_proj)? {
+ break;
+ }
+
// Collect the column references usage in the outer projection.
Review Comment:
Damn, this is tricky indeed... I think it's good 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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3865597007
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -71,18 +71,31 @@ pub trait ExprSchemable {
-> Result<(DataType, bool)>;
}
-/// Derives the output field for a cast expression from the source field.
+/// Derives the output field for a cast expression from the source and target
+/// fields. Type-only casts preserve source metadata, while an explicit target
+/// field supplies its own metadata.
+///
/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
fn cast_output_field(
source_field: &FieldRef,
- target_type: &DataType,
+ target_field: &FieldRef,
force_nullable: bool,
) -> Arc<Field> {
+ // `Cast::new` and `TryCast::new` use this field when only a target
+ // type is known. In that case, retain the source field's metadata.
+ let type_only_target = target_field.name().is_empty()
+ && target_field.is_nullable()
+ && target_field.metadata().is_empty();
+ let metadata = if type_only_target {
Review Comment:
mine can also be wrong. its kinda implicit. I think this difference should be properly marked but might be a larger change like with an enum:
```rust
enum CastTarget { DataType, Field }
```
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#issuecomment-5482422166
hey @timsaucer yes, this PR was originally meant to be stacked on #24725 but because of the breaking changes we are gong to take #23169 approach which avoids this for now. Then I will rebase this on that PR and will not hve these breaking change 👍
--
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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3917238090
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -1014,6 +1030,10 @@ pub fn remove_unnecessary_projections(
plan: Arc<dyn ExecutionPlan>,
) -> Result<Transformed<Arc<dyn ExecutionPlan>>> {
let maybe_modified = if let Some(projection) = plan.downcast_ref::<ProjectionExec>() {
+ // Removing a projection with observable metadata can change query results.
+ if projection.overrides_metadata()? {
+ return Ok(Transformed::no(plan));
+ }
Review Comment:
The cost concern here is solved with the changes that will be mae to `overrides_metadata` so i think we are good on that frohnt. It will now read the cached bool
For the condition removal, I don't believe we can do this. There are two checks that I think are being confused as checking the same thing:
1. In `is_projection_removable` we check if the schemas of the projuection and its input are a match and if they are we can remove it.
2. Then we check if we can swap the projection to be below its child, like in the filter example above. This is a differnt check and the guard is still needed.
Say you had this:
```text
Projection: a metadata={unit: ms}
Filter: arrow_metadata(a, "unit") IS NULL
Input: a,b metadata={}
```
After the swap:
```text
Filter: arrow_metadata(a, "unit") IS NULL
Projection: a metadata={unit: ms}
Input
```
The filter outputs the schema it receives so it now receives the schema with the metadata. Meaning that the proposed check will pass, but the filter now also evaluates its predicate using that metadata. Before this it saw rows with no metadata thus returns NULL and the rows pass. Then after it is inspecting it with metadata so the rows dont pass.
I don't know if this is accessible via SQL, but it defnitely can via public APIs. I added a test that checks for this 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: preserve projection metadata during optimization [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3905772751
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions' failed[\s\S]*TryCast from FixedSizeBinary\(16\) to FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T
Review Comment:
yes I think we are in aggreance, long term there is most likely a better solution than what is proposed. This PR is just trying to fix a bug that we saw pop up after the df55 upgrade and then created this cascading effect of more bugs being unveiled.
If we are ok with leaving that known incorrect behavior in the minor patch we can do the check:
> Does target_field have metadata? if yes, then use that metadata, if not, then use the source_field metadata.
Seems like it will be sorted in the meeting tmrw 🙇
--
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: preserve projection metadata during optimization [datafusion]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3927068478
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -270,6 +278,33 @@ impl ProjectionExec {
))
}
+ /// Returns whether `projector`'s output metadata differs from the metadata
+ /// derived from its expressions and `input_schema`.
+ fn compute_overrides_metadata(
+ projector: &Projector,
+ input_schema: &Schema,
+ ) -> Result<bool> {
+ let output_schema = projector.output_schema();
+ if input_schema.metadata() != output_schema.metadata() {
+ return Ok(true);
+ }
+ for (projection, output_field) in
+ projector.projection().iter().zip(output_schema.fields())
+ {
+ let derived_field = projection.expr.return_field(input_schema)?;
+ if derived_field.metadata() != output_field.metadata() {
+ return Ok(true);
+ }
+ }
+ Ok(false)
Review Comment:
Can we instead just reuse `projection.project_schema(input_schema)` and compare. the metadata?
##########
datafusion/physical-plan/src/projection.rs:
##########
@@ -83,6 +83,9 @@ pub struct ProjectionExec {
metrics: ExecutionPlanMetricsSet,
/// Cache holding plan properties like equivalences, output partitioning etc.
cache: Arc<PlanProperties>,
+ /// Whether the output metadata differs from the metadata derived from the
+ /// projection expressions and input schema.
+ overrides_metadata: bool,
Review Comment:
Agent picked up on something I overlooked: We are doing a `Clone` when we call `replace_children` so it's possible that `overrides_metadata` becomes inconsistent with the new children.
--
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: preserve projection metadata during optimization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #24670:
URL: https://github.com/apache/datafusion/pull/24670
--
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