Repository navigation
[pull] main from apache:main - #271
Merged
Merged
Conversation
…it at try_new + tighten Time variants (#22751) # Which issue does this PR close? PR 1 of 5 from the split agreed on #22706 (comment). Related to #22682 / #22715 (full `GroupValuesColumn` type coverage). Lays the dispatcher foundation; closes nothing on its own. # Rationale for this change Preparation for the nested-type `GroupColumn` work. No new builders here. The goal is to refactor the per-field builder dispatch in `GroupValuesColumn` so subsequent PRs that add `FixedSizeList` / `Struct` / `List` / `LargeList` support can plug into one well-defined factory instead of growing the inline match in `GroupValuesColumn::intern`. Also includes two adjacent correctness fixes around the `Time32` / `Time64` variants that came up in the upstream thread. # What changes are included in this PR 1. **Factory extraction.** The inline match that maps each schema field to a `Box<dyn GroupColumn>` builder moves out of `GroupValuesColumn::intern` into a free function `make_group_column(field: &Field) -> Result<Box<dyn GroupColumn>>`. Subsequent nested-type specializations can recursively call this factory for child field construction without enumerating every combination inline. 2. **Eager construction at `try_new`.** Per @2010YOUY01's review, the per-field builder vector is now built in the constructor via a private `build_group_columns` helper. `emit(EmitTo::All)` uses `mem::replace` to swap in a fresh vector after draining the old one; `clear_shrink` rebuilds the same way. The post-condition `self.group_values.len() == self.schema.fields().len()` holds across the aggregator's lifetime, so `intern` no longer carries a lazy-init branch. Unsupported schemas now fail fast at `try_new` rather than at the first `intern` call. In production this changes nothing because `new_group_values` in `aggregates/group_values/mod.rs` only calls `GroupValuesColumn::try_new` after `multi_group_by::supported_schema` returns true. 3. **`Time32` / `Time64` `supported_type` alignment.** Previously `supported_type` matched `Time32(_)` (admitting the invalid Microsecond / Nanosecond combinations) and did not match `Time64(_)` at all, while the dispatcher accepted `Time32(Second / Millisecond)` and `Time64(Microsecond / Nanosecond)`. Tighten `supported_type` to the exact set the dispatcher constructs. The dispatcher's wildcard arms for invalid `Time` variants now return `not_impl_err` instead of silently producing an empty builder vector. 4. **`supported_type` ↔ `make_group_column` consistency fuzz.** New unit test `supported_type_and_make_group_column_stay_in_sync` iterates a representative set of 20 supported and 6 unsupported `DataType` values and asserts the biconditional. Pins the alignment so future contributors who add a type to one side without the other trip a unit test immediately. # What this PR is NOT doing - No new `GroupColumn` builders. `FixedSizeList`, `Struct`, `List`, `LargeList` come in subsequent PRs of the #22682 sequence. - No new types in `supported_type`'s allow-list (the `Time` tightening removes invalid combinations rather than adding new ones). - No external dependencies. Per @alamb's review the earlier `dhat-heap` feature and the `dhat` dependency were dropped; memory savings in follow-up PRs will be measured via `GroupColumn::size()` head-to-head against `GroupValuesRows`, matching the existing `column_path_uses_less_memory_than_rows_for_*` tests pattern in #22706. # Are these changes tested - `cargo test -p datafusion-physical-plan --lib aggregates::group_values`: 30 tests pass (27 existing + 3 new: the consistency fuzz, the mixed-schema rejection, and the `try_new`-time NotImpl propagation). - `cargo test -p datafusion-physical-plan --lib aggregates`: 105 tests pass (no regression in the broader aggregate suite). - `cargo clippy -p datafusion-physical-plan --lib --tests -- -D warnings`: clean. - `cargo fmt --check`: clean. # Are there any user-facing changes No semantic change for any schema that already used `GroupValuesColumn` on main. The factory is the same dispatch logic pulled into a function; the `Time` changes only affect schemas that are semantically invalid Arrow types anyway; the eager construction surfaces `not_impl_err` from a different call site for those defensive paths.
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - Closes #22818. ## Rationale for this change DataFusion kept SQL dialect metadata in several separate places, which made it easy for enum variants, canonical names, aliases, display names, docs, and error messages to drift. <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> ## What changes are included in this PR? - Centralize dialect metadata in one macro-driven source. - Generate the `Dialect` enum, metadata table, canonical names, aliases, display names, and available-dialect string from that source. - Replace `Dialect::AVAILABLE` with `Dialect::available()`. - Add `Dialect::metadata()` for iterating supported dialects. - Update dialect error messages and generated config docs to use the centralized metadata. - Add an upgrade note for the removed public constant. <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? Yes <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? Yes. `Dialect::AVAILABLE` is removed and replaced by `Dialect::available()`. <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )