Copilot commented on code in PR #26938:
URL: https://github.com/apache/datafusion/pull/26938#discussion_r4426007254
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -507,6 +521,24 @@ from table_with_metadata limit 1;
----
NULL the id field
+# Test MAX window function preserves metadata
+query IT
+select
+ max(id) over (order by id asc nulls last) as mx,
+ arrow_metadata(max(id) over (order by id asc nulls last), 'metadata_key') as
meta
+from table_with_metadata limit 1;
Review Comment:
These window-function sqllogictest cases are potentially nondeterministic:
`LIMIT 1` without an outer `ORDER BY` can return any row, and `mx` will vary
depending on which row is returned (e.g., could be 1, 2, or 3). To make the
expected output stable, add an outer `ORDER BY id asc nulls last` before `LIMIT
1`, or filter to a specific row (e.g., `where id = 1`). Apply the same fix to
the corresponding `min(...) over (...) ... limit 1` query.
##########
datafusion/functions-aggregate/src/min_max.rs:
##########
@@ -220,6 +223,13 @@ impl AggregateUDFImpl for Max {
Ok(arg_types[0].to_owned())
}
+ fn return_field(&self, arg_fields: &[FieldRef]) -> Result<FieldRef> {
+ Ok(Arc::new(
+ Field::new(self.name(), arg_fields[0].data_type().clone(), true)
+ .with_metadata(arg_fields[0].metadata().clone()),
Review Comment:
Constructing a brand-new `Field` here may unintentionally drop other
important `Field` properties beyond `metadata` (e.g., dictionary id / ordering
or any other `Field`-level attributes supported by the Arrow version in use).
To avoid losing information, prefer cloning the input field and only adjusting
what must change (name to `self.name()` and nullability to `true`), while
preserving all other `Field` attributes. The same issue applies to
`Min::return_field`.
##########
datafusion/functions-aggregate/src/min_max.rs:
##########
@@ -220,6 +223,13 @@ impl AggregateUDFImpl for Max {
Ok(arg_types[0].to_owned())
}
+ fn return_field(&self, arg_fields: &[FieldRef]) -> Result<FieldRef> {
+ Ok(Arc::new(
+ Field::new(self.name(), arg_fields[0].data_type().clone(), true)
+ .with_metadata(arg_fields[0].metadata().clone()),
+ ))
+ }
Review Comment:
`Max::return_field` and `Min::return_field` now contain effectively
identical logic. Consider extracting a small shared helper (e.g., a private
function that takes `name` and `arg_fields[0]`) to keep the behavior consistent
and reduce the chance of future drift when adjusting field/metadata
preservation 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
