cdelmonte-zg opened a new pull request, #25196: URL: https://github.com/apache/datafusion/pull/25196
## Which issue does this PR close? * Closes #17715. ## Rationale for this change Partial aggregate state schemas can contain duplicate field names when multiple order-sensitive aggregate expressions use the same ordering expression. For example, `first_value(value ORDER BY timestamp)` and `last_value(value ORDER BY timestamp)` both exposed the ordering state field as `timestamp@0`. This required `DFSchema::try_from` to skip `check_names()` as a workaround. However, the `state_fields()` contract requires state field names to be unique within the query. ## What changes are included in this PR? * Namespace ordering state fields with the aggregate name and the ordering-field position. * Apply the same naming scheme to the default `AggregateUDFImpl::state_fields()` implementation and to `FirstValue` / `LastValue`. * Re-enable `DFSchema::check_names()` in `TryFrom<SchemaRef> for DFSchema`. * Update the partial aggregate regression test to assert unique state field names. * Add a unit test for the default `state_fields()` implementation, including duplicate ordering field names within a single aggregate. For example, an ordering state field now has a name such as: ```text first_value(value)[ordering_0_timestamp@0] ``` rather than the unqualified: ```text timestamp@0 ``` The ordering position also disambiguates repeated ordering fields within the same aggregate. ## What is the testing strategy for this PR? The change is covered by: * `test_partial_aggregate_state_fields_have_unique_names` * `test_default_state_fields_namespaces_ordering_fields` The following test suites pass locally: ```text cargo test -p datafusion-expr cargo test -p datafusion-functions-aggregate cargo test -p datafusion-physical-plan cargo test -p datafusion-common cargo test -p datafusion --test core_integration ``` `cargo fmt --all -- --check` and `git diff --check` also pass. ## Are there any user-facing changes? No public API changes are introduced. Partial aggregate state field names are now generated uniquely, allowing `DFSchema::check_names()` to be re-enabled. -- 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]
