preserve Field metadata in first_value/last_value by adriangb · Pull Request #19335 · apache/datafusion

adriangb · 2025-12-15T16:04:23Z

Which issue does this PR close?

Closes #19336

Rationale for this change

The first_value and last_value aggregate functions were not preserving Field metadata from their input arguments. This caused metadata to be lost when using these functions, which affects downstream consumers that rely on metadata (e.g., for DISTINCT ON queries which use first_value internally).

What changes are included in this PR?

Implement return_field() for FirstValue to preserve input field metadata
Implement return_field() for LastValue to preserve input field metadata
Add get_metadata UDF for testing metadata preservation in sqllogictest
Add regression tests for first_value, last_value, DISTINCT ON, DISTINCT, and grouped columns

Are these changes tested?

Yes, new sqllogictest tests are added in metadata.slt that verify metadata is preserved through various aggregate operations.

Are there any user-facing changes?

Yes, Field metadata is now correctly preserved when using first_value() and last_value() aggregate functions. This is a bug fix that improves metadata propagation.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.5 [email protected]

Jefffrey · 2025-12-15T23:15:40Z

datafusion/functions-aggregate/src/first_last.rs

@@ -137,6 +137,18 @@ impl AggregateUDFImpl for FirstValue {
        Ok(arg_types[0].clone())


Ideally we should have return_type return an internal error instead of leaving it implemented if we use return_field now

Jefffrey · 2025-12-15T23:15:55Z

datafusion/functions-aggregate/src/first_last.rs

@@ -1075,6 +1087,18 @@ impl AggregateUDFImpl for LastValue {
        Ok(arg_types[0].clone())


Jefffrey · 2025-12-15T23:23:52Z

datafusion/sqllogictest/src/test_context.rs

+/// 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 {


I wonder if this is useful enough to introduce as a function to datafusion itself, instead of being only in SLT? 🤔

Happy to do so if you think it's worth it - I went with the conservative approach

We don't need to do it in this PR, but worth filing a ticket for as followup

Jefffrey · 2025-12-15T23:25:26Z

datafusion/sqllogictest/test_files/metadata.slt

 3 1
 NULL 1

+# Regression test: first_value should preserve metadata


I noticed for the existing regression tests in this file, they don't actually check metadata 🤔

datafusion/datafusion/sqllogictest/test_files/metadata.slt

Lines 63 to 67 in 58377bf

# Regression test: prevent field metadata loss per https://github.com/apache/datafusion/issues/12687

query I

select count(distinct name) from table_with_metadata;

----

2

With this new UDF we can look into updating those tests to be more similar to the ones introduced here

Will file a ticket before we merge this to do so

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

> select arrow_metadata('foo');

I think this would help debug various metadata issues more easily

I can file a ticket if you think this is reasonable

alamb

Thank you @adriangb -- I agree with all @Jefffrey 's comments too

Let me know if you want a follow up ticket

alamb · 2025-12-16T15:23:38Z

datafusion/sqllogictest/test_files/metadata.slt

 3 1
 NULL 1

+# Regression test: first_value should preserve metadata


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

> select arrow_metadata('foo');

alamb · 2025-12-16T15:24:04Z

datafusion/sqllogictest/test_files/metadata.slt

 3 1
 NULL 1

+# Regression test: first_value should preserve metadata


I think this would help debug various metadata issues more easily

I can file a ticket if you think this is reasonable

alamb · 2025-12-16T15:24:56Z

datafusion/sqllogictest/src/test_context.rs

+    }
+
+    fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> {
+        // Get the metadata key from the second argument (must be a string literal)


I think it would also be nice if we supported a single column version that returned the metadata as a struct array too

martin-g · 2025-12-16T16:16:22Z

datafusion/sqllogictest/test_files/metadata.slt

+----
+1 the id field NULL the name field
+3 the id field baz the name field
+NULL the id field bar the name field


What would happen if a non-existing column is passed as a first argument of the get_metadata() udf ? Or a scalar value.

Return an empty map?

Or NULL.
I mean it would be good to have some negative test cases too.

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

adriangb · 2025-12-16T16:47:52Z

I opened #19356 to track generalizing the function added in this PR.

alamb · 2025-12-17T15:54:59Z

@erratic-pattern is looking at something similar for us upstream in InfluxDB

preserve Field metadata in first_value/last_value

f13ffd7

github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Dec 15, 2025

This was referenced Dec 15, 2025

[EPIC] Schema metadata handling / bugs #12733

Closed

Document Schema metadata expectations #12736

Open

adriangb requested a review from alamb December 15, 2025 16:09

adriangb added a commit to pydantic/datafusion that referenced this pull request Dec 15, 2025

preserve Field metadata in first_value/last_value apache#19335

74b2c79

make deterministic

76b465a

Jefffrey reviewed Dec 15, 2025

View reviewed changes

claude bot mentioned this pull request Dec 16, 2025

19335: preserve Field metadata in first_value/last_value martin-augment/datafusion#126

Open

alamb approved these changes Dec 16, 2025

View reviewed changes

martin-g reviewed Dec 16, 2025

View reviewed changes

adriangb added 2 commits December 16, 2025 10:29

return not impl err from return_type

89a0bc3

fmt

5b21465

adriangb mentioned this pull request Dec 16, 2025

Add arrow_metadata(<expr>) UDF #19356

Closed

adriangb added this pull request to the merge queue Dec 16, 2025

Merged via the queue into apache:main with commit 50d20dd Dec 16, 2025
27 checks passed

		@@ -137,6 +137,18 @@ impl AggregateUDFImpl for FirstValue {
		Ok(arg_types[0].clone())

		@@ -1075,6 +1087,18 @@ impl AggregateUDFImpl for LastValue {
		Ok(arg_types[0].clone())

	# Regression test: prevent field metadata loss per https://github.com/apache/datafusion/issues/12687
	query I
	select count(distinct name) from table_with_metadata;
	----
	2

Comments

Conversation

adriangb commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb commented Dec 16, 2025

Uh oh!

Uh oh!

alamb commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adriangb commented Dec 15, 2025 •

edited

Loading