[fix](be) Use serialized hash keys for complex types - #66777
Open
Mryange wants to merge 1 commit into
Open
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Multi-column hash keys classified fixed-width STRUCT columns as fixed keys based only on their maximum in-memory size. ColumnStruct stores fields in separate child columns, so fixed-key packing either rejected the computed key size or attempted to read a non-contiguous raw buffer. Use the existing serialized-key path whenever a multi-column key contains a complex type.
### Release note
Fix GROUP BY and DISTINCT aggregate failures when a grouped key contains a fixed-width STRUCT column.
### Check List (For Author)
- Test: Regression test
- ./run-regression-test.sh --run -d datatype_p0/complex_types -s test_group_by_fixed_width_struct
- Unit Test: Added HashKeyTypeTest coverage; not run because the existing BEUT binary predates the new test file
- Behavior changed: Yes, complex types in multi-column hash keys use serialized keys
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review status: Changes requested
I found one blocking data-correctness issue: the new serialized path does not preserve Doris FLOAT/DOUBLE equality at every hash boundary for fixed-width STRUCT keys. See the inline comment for the concrete signed-zero INTERSECT failure.
Critical checkpoints:
- Goal and proof: The change correctly prevents fixed-key raw-buffer packing for integer fixed-width STRUCT values, and the selector/unit cases plus deterministic GROUP BY/DISTINCT regression cover that repaired path. The goal is not complete for STRUCT values containing FLOAT/DOUBLE because semantically equal representations can still hash differently.
- Scope and clarity: The implementation change is small and focused, and ordinary scalar fixed-key selection remains unchanged.
- End-to-end and parallel paths: Aggregation, streaming/bucketed/distinct aggregation, joins, set operations, partition sort, recursive-CTE deduplication, and complex hash-map dictionaries all use the central selector. Exchange/local partition hashing and several local serialized-key consumers use raw nested float bits, which is the accepted issue.
- Concurrency, lifecycle, memory, and errors: This is initialization-time method selection with no new shared mutable state, locks, static lifecycle, or error-suppression path. Existing serialized arenas retain key storage through their consumers; no separate ownership issue was found.
- Compatibility, configuration, persistence, and writes: No protocol/storage-format, rolling-upgrade, configuration, transaction, persistence, or FE-BE variable change is introduced.
- Tests: The BE unit is discovered by the test CMake glob and directly exercises selector behavior; the commit records the targeted regression as run. The new tests use only integral STRUCT fields and miss signed zero/NaN across set/hash-distribution paths. This review runner did not build or run tests, per its instructions.
- Performance and observability: Scalar fixed-key fast paths are preserved; serialized keys are necessary for non-contiguous STRUCT storage. No separate performance regression or new metric/log requirement was substantiated.
- User focus: No additional review focus was supplied.
Review completion: Converged in Round 2. One inline finding is accepted; its corroborating candidate was deduplicated, and no other suspicious point remains unresolved.
|
|
||
| for (const auto& data_type : data_types) { | ||
| if (!data_type->have_maximum_size_of_value()) { | ||
| if (is_complex_type(data_type->get_primitive_type()) || |
Contributor
There was a problem hiding this comment.
[P1] Preserve FLOAT equality at every STRUCT hash boundary
This newly sends a multi-column key such as (id, STRUCT<DOUBLE>) through serialized-key consumers, but nested floats are not canonicalized at every hash boundary. ColumnStruct preserves raw IEEE bits in both exchange CRC hashing and MethodSerialized, so (1, {-0.0}) and (1, {+0.0}) can route to different set instances or produce different local StringRefs even though CompareFloat() defines them as equal; INTERSECT can consequently drop the matching row (distinct NaN payloads have the same issue). Please canonicalize early enough for exchange partitioning and every local build/probe/partition/dedup path to consume the same semantic representation, and add a multi-key STRUCT float regression.
Contributor
Author
|
/review 重新review 下， flaot这个问题过去就有 ，我们倾向于搞一个单独的pr。 |
Contributor
There was a problem hiding this comment.
Automated review result: no new valuable findings. This is a comment-only review with zero new inline comments.
Round 1 converged: both normal full-scope reviewers and the separate risk-focused reviewer returned NO_NEW_VALUABLE_FINDINGS, and the main review independently resolved every recorded risk.
Critical checkpoint conclusions:
- Goal and correctness: the change correctly prevents multi-column fixed-width
STRUCTkeys from enteringMethodKeysFixed, whose raw-buffer packing is incompatible withColumnStruct's non-contiguous child-column layout. The existing typed serialized path preserves nullable markers, build/probe equality, owning-arena lifetimes, and output reconstruction. - Scope and clarity: the production change is a focused selector guard that reuses existing infrastructure.
ARRAYandMAPwere already serialized, nullable types delegate their primitive classification, single complex keys were already serialized, and scalar/VARIANT/fixed-size routing remains unchanged. - Parallel paths and error handling: aggregation (ordinary, streaming, bucketed, spill/phase 2), distinct/recursive CTE, hash join, INTERSECT/EXCEPT, and partition-sort variants all support
HashKeyType::serialized. Their insertion, probe, output, and exception-to-Statuspaths were traced; no incompatible or silently failing path was found. - Concurrency and lifecycle: no new thread, lock, atomic, static initialization, or shared-state lifecycle is introduced. Serialized build keys are copied into or retained by the existing owning arenas; transient probe buffers are not persisted.
- Configuration, compatibility, persistence, and writes: no configuration, FE-BE protocol, storage format, EditLog, transaction, or data-write behavior changes. Hash-key representation remains operator-local, so there is no rolling-upgrade wire-format dependency.
- Performance and observability: serialization adds the necessary per-row arena work only for fixed-width
STRUCTlayouts that cannot safely use fixed packing. No new untracked allocation, pathological loop, long-running operation, log, or metric requirement was found. - Tests and results: the selector unit test is glob-registered by BE test CMake; current PR checks show BE UT (macOS) and Clang Formatter passing. The focused regression covers the formerly invalid multi-key GROUP BY layouts plus three DISTINCT aggregate paths, uses deterministic ordering, and its checked-in cardinalities match the inserted rows. The commit records the focused regression command as run; this review did not rerun builds/tests because the runner instructions prohibit builds.
User focus — FLOAT: I re-reviewed the raw IEEE equality concern. It already exists in the unchanged single-STRUCT serialized path (and related pre-existing hash boundaries), is already covered by the existing inline thread, and the user prefers a separate PR. I found no distinct FLOAT regression or other equality bug to comment on here, so I did not duplicate that thread.
Review completion status: converged in Round 1; no unresolved candidates and no new inline findings.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17661 ms |
Contributor
TPC-DS: Total hot run time: 85922 ms |
Contributor
ClickBench: Total hot run time: 14.71 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What problem does this PR solve?
Problem Summary: Multi-column hash keys could incorrectly classify fixed-width complex types such as
STRUCTas fixed keys. BecauseColumnStructstores its fields in separate child columns, it cannot be packed from a contiguous raw buffer. This change routes multi-column keys containing complex types through the serialized-key path, fixingGROUP BYand distinct aggregate failures involving fixed-widthSTRUCTcolumns.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)