You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2025/12/15 16:04:25 UTC
[PR] preserve Field metadata in first_value/last_value [datafusion]
adriangb opened a new pull request, #19335:
URL: https://github.com/apache/datafusion/pull/19335
(no comment)
--
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] preserve Field metadata in first_value/last_value [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#discussion_r2621155616
##########
datafusion/functions-aggregate/src/first_last.rs:
##########
@@ -1075,6 +1087,18 @@ impl AggregateUDFImpl for LastValue {
Ok(arg_types[0].clone())
Review Comment:
Ditto
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -235,7 +235,56 @@ order by 1 asc nulls last;
3 1
NULL 1
+# Regression test: first_value should preserve metadata
Review Comment:
I noticed for the existing regression tests in this file, they don't actually check metadata 🤔
https://github.com/apache/datafusion/blob/58377bf2e30315438f6cff047874c3fac770f31e/datafusion/sqllogictest/test_files/metadata.slt#L63-L67
With this new UDF we can look into updating those tests to be more similar to the ones introduced here
##########
datafusion/sqllogictest/src/test_context.rs:
##########
@@ -398,6 +398,58 @@ pub async fn register_metadata_tables(ctx: &SessionContext) {
.unwrap();
ctx.register_batch("table_with_metadata", batch).unwrap();
+
+ // Register the get_metadata UDF for testing metadata preservation
+ ctx.register_udf(ScalarUDF::from(GetMetadataUdf::new()));
+}
+
+/// UDF to extract metadata from a field for testing purposes
+/// Usage: get_metadata(expr, 'key') -> returns the metadata value or NULL
+#[derive(Debug, PartialEq, Eq, Hash)]
+struct GetMetadataUdf {
Review Comment:
I wonder if this is useful enough to introduce as a function to datafusion itself, instead of being only in SLT? 🤔
##########
datafusion/functions-aggregate/src/first_last.rs:
##########
@@ -137,6 +137,18 @@ impl AggregateUDFImpl for FirstValue {
Ok(arg_types[0].clone())
Review Comment:
Ideally we should have `return_type` return an internal error instead of leaving it implemented if we use `return_field` 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] preserve Field metadata in first_value/last_value [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#discussion_r2623918755
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -235,7 +235,56 @@ order by 1 asc nulls last;
3 1
NULL 1
+# Regression test: first_value should preserve metadata
+query IT
+select first_value(id order by id asc nulls last), get_metadata(first_value(id order by id asc nulls last), 'metadata_key')
+from table_with_metadata;
+----
+1 the id field
+
+# Regression test: last_value should preserve metadata
+query IT
+select last_value(id order by id asc nulls first), get_metadata(last_value(id order by id asc nulls first), 'metadata_key')
+from table_with_metadata;
+----
+3 the id field
+# Regression test: DISTINCT ON should preserve metadata (uses first_value internally)
+query ITTT
+select distinct on (id) id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from table_with_metadata order by id asc nulls last;
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
+
+# Regression test: DISTINCT should preserve metadata
+query ITTT
+with res AS (
+ select distinct id, name from table_with_metadata
+)
+select id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from res
+order by id asc nulls last;
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
+
+# Regression test: grouped columns should preserve metadata
+query ITTT
+with res AS (
+ select name, count(*), id
+ from table_with_metadata
+ group by id, name
+)
+select id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from res
+order by id asc nulls last, name asc nulls last
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
Review Comment:
What would happen if a non-existing column is passed as a first argument of the get_metadata() udf ? Or a scalar value.
--
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] preserve Field metadata in first_value/last_value [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#discussion_r2621664436
##########
datafusion/sqllogictest/src/test_context.rs:
##########
@@ -398,6 +398,58 @@ pub async fn register_metadata_tables(ctx: &SessionContext) {
.unwrap();
ctx.register_batch("table_with_metadata", batch).unwrap();
+
+ // Register the get_metadata UDF for testing metadata preservation
+ ctx.register_udf(ScalarUDF::from(GetMetadataUdf::new()));
+}
+
+/// UDF to extract metadata from a field for testing purposes
+/// Usage: get_metadata(expr, 'key') -> returns the metadata value or NULL
+#[derive(Debug, PartialEq, Eq, Hash)]
+struct GetMetadataUdf {
Review Comment:
We don't need to do it in this PR, but worth filing a ticket for as followup
--
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] preserve Field metadata in first_value/last_value [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #19335:
URL: https://github.com/apache/datafusion/pull/19335
--
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] preserve Field metadata in first_value/last_value [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#discussion_r2623947695
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -235,7 +235,56 @@ order by 1 asc nulls last;
3 1
NULL 1
+# Regression test: first_value should preserve metadata
+query IT
+select first_value(id order by id asc nulls last), get_metadata(first_value(id order by id asc nulls last), 'metadata_key')
+from table_with_metadata;
+----
+1 the id field
+
+# Regression test: last_value should preserve metadata
+query IT
+select last_value(id order by id asc nulls first), get_metadata(last_value(id order by id asc nulls first), 'metadata_key')
+from table_with_metadata;
+----
+3 the id field
+# Regression test: DISTINCT ON should preserve metadata (uses first_value internally)
+query ITTT
+select distinct on (id) id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from table_with_metadata order by id asc nulls last;
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
+
+# Regression test: DISTINCT should preserve metadata
+query ITTT
+with res AS (
+ select distinct id, name from table_with_metadata
+)
+select id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from res
+order by id asc nulls last;
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
+
+# Regression test: grouped columns should preserve metadata
+query ITTT
+with res AS (
+ select name, count(*), id
+ from table_with_metadata
+ group by id, name
+)
+select id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from res
+order by id asc nulls last, name asc nulls last
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
Review Comment:
Return an empty map?
--
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] preserve Field metadata in first_value/last_value [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#issuecomment-3661474268
I opened https://github.com/apache/datafusion/issues/19356 to track generalizing the function added in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] preserve Field metadata in first_value/last_value [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#discussion_r2621582323
##########
datafusion/sqllogictest/src/test_context.rs:
##########
@@ -398,6 +398,58 @@ pub async fn register_metadata_tables(ctx: &SessionContext) {
.unwrap();
ctx.register_batch("table_with_metadata", batch).unwrap();
+
+ // Register the get_metadata UDF for testing metadata preservation
+ ctx.register_udf(ScalarUDF::from(GetMetadataUdf::new()));
+}
+
+/// UDF to extract metadata from a field for testing purposes
+/// Usage: get_metadata(expr, 'key') -> returns the metadata value or NULL
+#[derive(Debug, PartialEq, Eq, Hash)]
+struct GetMetadataUdf {
Review Comment:
Happy to do so if you think it's worth it - I went with the conservative approach
--
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] preserve Field metadata in first_value/last_value [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#discussion_r2623959336
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -235,7 +235,56 @@ order by 1 asc nulls last;
3 1
NULL 1
+# Regression test: first_value should preserve metadata
+query IT
+select first_value(id order by id asc nulls last), get_metadata(first_value(id order by id asc nulls last), 'metadata_key')
+from table_with_metadata;
+----
+1 the id field
+
+# Regression test: last_value should preserve metadata
+query IT
+select last_value(id order by id asc nulls first), get_metadata(last_value(id order by id asc nulls first), 'metadata_key')
+from table_with_metadata;
+----
+3 the id field
+# Regression test: DISTINCT ON should preserve metadata (uses first_value internally)
+query ITTT
+select distinct on (id) id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from table_with_metadata order by id asc nulls last;
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
+
+# Regression test: DISTINCT should preserve metadata
+query ITTT
+with res AS (
+ select distinct id, name from table_with_metadata
+)
+select id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from res
+order by id asc nulls last;
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
+
+# Regression test: grouped columns should preserve metadata
+query ITTT
+with res AS (
+ select name, count(*), id
+ from table_with_metadata
+ group by id, name
+)
+select id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from res
+order by id asc nulls last, name asc nulls last
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
Review Comment:
Or NULL.
I mean it would be good to have some negative test cases too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] preserve Field metadata in first_value/last_value [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#discussion_r2623998142
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -235,7 +235,56 @@ order by 1 asc nulls last;
3 1
NULL 1
+# Regression test: first_value should preserve metadata
+query IT
+select first_value(id order by id asc nulls last), get_metadata(first_value(id order by id asc nulls last), 'metadata_key')
+from table_with_metadata;
+----
+1 the id field
+
+# Regression test: last_value should preserve metadata
+query IT
+select last_value(id order by id asc nulls first), get_metadata(last_value(id order by id asc nulls first), 'metadata_key')
+from table_with_metadata;
+----
+3 the id field
+# Regression test: DISTINCT ON should preserve metadata (uses first_value internally)
+query ITTT
+select distinct on (id) id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from table_with_metadata order by id asc nulls last;
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
+
+# Regression test: DISTINCT should preserve metadata
+query ITTT
+with res AS (
+ select distinct id, name from table_with_metadata
+)
+select id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from res
+order by id asc nulls last;
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
+
+# Regression test: grouped columns should preserve metadata
+query ITTT
+with res AS (
+ select name, count(*), id
+ from table_with_metadata
+ group by id, name
+)
+select id, get_metadata(id, 'metadata_key'), name, get_metadata(name, 'metadata_key')
+from res
+order by id asc nulls last, name asc nulls last
+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field
Review Comment:
> What would happen if a non-existing column is passed as a first argument of the get_metadata() udf
I think it should probably error like any other query that tries to access a undefined column
--
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] preserve Field metadata in first_value/last_value [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#discussion_r2621582618
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -235,7 +235,56 @@ order by 1 asc nulls last;
3 1
NULL 1
+# Regression test: first_value should preserve metadata
Review Comment:
Will file a ticket before we merge this to do so
--
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] preserve Field metadata in first_value/last_value [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#discussion_r2623746392
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -235,7 +235,56 @@ order by 1 asc nulls last;
3 1
NULL 1
+# Regression test: first_value should preserve metadata
Review Comment:
I think this would help debug various metadata issues more easily
I can file a ticket if you think this is reasonable
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -235,7 +235,56 @@ order by 1 asc nulls last;
3 1
NULL 1
+# Regression test: first_value should preserve metadata
Review Comment:
the tests I think test that there is metadata on the input tables (rather than the output tables)
I do really like the idea of adding a UDF, simlarly to `arrow_typeof` that can show the metadata
``sql
> select arrow_typeof('foo');
+---------------------------+
| arrow_typeof(Utf8("foo")) |
+---------------------------+
| Utf8 |
+---------------------------+
1 row(s) fetched.
Elapsed 0.024 seconds.
```
Possibilities: add a new argument
```sql
> select arrow_typeof('foo', true);
```
Possibility: add a new function
```sql
> select arrow_metadata('foo');
```
##########
datafusion/sqllogictest/src/test_context.rs:
##########
@@ -398,6 +398,58 @@ pub async fn register_metadata_tables(ctx: &SessionContext) {
.unwrap();
ctx.register_batch("table_with_metadata", batch).unwrap();
+
+ // Register the get_metadata UDF for testing metadata preservation
+ ctx.register_udf(ScalarUDF::from(GetMetadataUdf::new()));
+}
+
+/// UDF to extract metadata from a field for testing purposes
+/// Usage: get_metadata(expr, 'key') -> returns the metadata value or NULL
+#[derive(Debug, PartialEq, Eq, Hash)]
+struct GetMetadataUdf {
+ signature: Signature,
+}
+
+impl GetMetadataUdf {
+ fn new() -> Self {
+ Self {
+ signature: Signature::any(2, Volatility::Immutable),
+ }
+ }
+}
+
+impl ScalarUDFImpl for GetMetadataUdf {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn name(&self) -> &str {
+ "get_metadata"
+ }
+
+ fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
+ Ok(DataType::Utf8)
+ }
+
+ fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
+ // Get the metadata key from the second argument (must be a string literal)
Review Comment:
I think it would also be nice if we supported a single column version that returned the metadata as a struct array too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] preserve Field metadata in first_value/last_value [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #19335:
URL: https://github.com/apache/datafusion/pull/19335#issuecomment-3665994154
@erratic-pattern is looking at something similar for us upstream in InfluxDB
--
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