Conversation
zghong
requested review from
924060929,
CalvinKirs,
dataroaring,
englefly,
gavinchou,
morningman,
morrySnow,
starocean999,
xiaokang and
yiguolei
as code owners
August 5, 2026 07:28
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29153 ms |
Contributor
TPC-DS: Total hot run time: 166066 ms |
Contributor
ClickBench: Total hot run time: 23.98 s |
…ion_hash_type identity
…ash_type # Conflicts: # be/src/exec/partitioner/partitioner.h # fe/fe-core/src/main/java/org/apache/doris/planner/HashDistributionPruner.java # fe/fe-core/src/test/java/org/apache/doris/planner/HashDistributionPrunerTest.java
Carry the effective storage hash type through FE-planned and BE-native local bucket exchanges. Add fragment protocol metadata and execution-version gating so older backends cannot silently execute IDENTITY layouts as CRC32.
Author
|
run buildall |
Restore compares table signatures using only intersecting partition names. When the intersection is empty, an IDENTITY partition can be appended to a CRC32 table, or vice versa. Subsequent writes use the table default hash while tablet pruning uses the partition hash, so committed rows can be missed. Include the non-CRC32 table default hash independently of the partition list. Preserve legacy CRC32/random signatures and allow different bucket counts when the hash algorithm matches. Reuse existing restore schema mismatch and force-replacement handling. Reject restoring partitions into an existing table with an incompatible distribution hash algorithm, including when partition names do not overlap.
Problem Summary: Identity hash tests could pass without exercising the intended distribution paths: mixed-hash joins could broadcast, set-operation settings excluded storage bucket shuffle, and zero high bytes hid wide-value truncation. Force and assert ordinary PARTITIONED joins, verify set-operation basic-side properties and remote/local thrift hash fields, and use independent arbitrary-precision high-byte, seed and NULL-tail vectors. Also verify that the local-exchange selector creates the identity partitioner, force multiple local channels, cover legacy-planner pruning, and assert the exact SHOW CREATE hash property. Generate regression golden results through the test runner and verify them with an independent result oracle.
Problem Summary: Older remote-Doris FEs ignore the new distribution hash type in table metadata and prune identity buckets with CRC32. A BIGINT value of 1 in an eight-bucket identity table is stored in bucket 1, but a metadata-version-140 client selects bucket 7 and misses the committed row. The same CRC32 table is read correctly. Reject identity metadata exports to clients without version 141 support before copying table or partition metadata. Return an empty required table_meta field so the error survives Thrift serialization. Keep CRC32 and random-distribution exports unchanged. Remote Doris clients with missing metadata versions or versions below 141 receive an explicit upgrade error when accessing identity tables, instead of silently pruning the wrong tablets. CRC32 tables are unaffected.
…ash_type # Conflicts: # be/src/agent/be_exec_version_manager.cpp # be/src/agent/be_exec_version_manager.h # fe/fe-common/src/main/java/org/apache/doris/common/Config.java # fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java # fe/fe-core/src/test/java/org/apache/doris/planner/LocalShuffleNodeCoverageTest.java
Problem Summary: Keep static-analysis adaptations limited to the identity hash implementation and its tests: equivalent scoped locking, uppercase unsigned literals, test byte construction, and precise documented diagnostic exceptions. Preserve test vectors and assertions. The unknown-wire enum fixture's exception does not establish defined behavior for its invalid cast. Unrelated clang-tidy tooling and core header changes are excluded.
zghong
force-pushed
the
feat/distribution_hash_type
branch
from
September 18, 2026 09:36
8c124d7 to
082b86b
Compare
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 27394 ms |
Contributor
TPC-DS: Total hot run time: 153651 ms |
Contributor
ClickBench: Total hot run time: 24.93 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of exact head 082b86b9b247dca1b795cfae311acf8f09a1293a. Requesting changes for two P1 correctness/lifecycle defects and one P2 resource-amplification issue.
Checkpoint conclusions:
- Goal and scope: the PR broadly implements persisted IDENTITY bucket distribution across FE metadata/planning and BE routing, and the large cross-module change remains cohesive around that goal.
- Data correctness and parallel paths: tablet routing, static pruning, load, remote exchange, both local-exchange planners, and the other supported canonical encodings agree. The remote legacy-DATE runtime-filter path does not, and can false-prune a matching bucket.
- Concurrency and lifecycle: the cache mutex serializes cache builders, but it does not synchronize with
merge(). Publishing a merger-owned wrapper to an ordinary local consumer can trip the merge invariant or race exact-set mutation against iteration. No separate lock-order or deadlock defect survived review. - Compatibility, configuration, and persistence: metadata version 141 and BE execution version 16 gates fail closed for IDENTITY, while absent fields preserve CRC32. CREATE/ADD/automatic partitioning, copies, replay, colocate validation, restore, and remote export otherwise retain or reject the new layout consistently; no new dynamic configuration surface was introduced.
- Performance and memory: the per-bucket-count cache retains a value-count-sized vector and repeats hashing for every distinct selected bucket count, with no cache-level bound or eviction.
- Tests: the changed unit/regression coverage is broad, but it does not cover remote DATE reconstruction, mixed local-merge/ordinary-local scheduling, or many selected bucket counts.
- Error handling, observability, security, and writes: no additional error-contract, logging/metrics, authentication/authorization, outbound-network, transaction, or write-atomicity defect survived the full call-chain review.
The supplied focus file contained no additional focus. All 86 authoritative changed paths and all candidates were swept through two rounds; every convergence reviewer returned NO_NEW_VALUABLE_FINDINGS. This was a static review only: I did not run builds or tests, and treated PR/CI results as external evidence rather than independent validation.
| it != _identity_bucket_prune_hashes.end()) { | ||
| return it->second; | ||
| } | ||
| _bucket_prune_hashes_started.store(true); |
Contributor
There was a problem hiding this comment.
[P1] Do not materialize this cache on a wrapper that can still be merged. With multiple producers and both local-merge and ordinary local targets, the first merge_from() stores the producer's shared wrapper in the unfinished merger, then publish() signals ordinary local consumers with that same wrapper. If pruning reaches this store before the next producer, merge() hits DORIS_CHECK(!_bucket_prune_hashes_started); if it overlaps just after that check, merge mutates _hybrid_set while this branch iterates it. Please delay exposure until the merger is complete or publish an independent immutable snapshot, and add a mixed-target scheduling test.
| primitive_type, 0, bucket_num)); | ||
| } else { | ||
| buckets->push_back( | ||
| RawValue::identity_hash(value, 0, primitive_type, 0, bucket_num)); |
Contributor
There was a problem hiding this comment.
[P1] Normalize remote DATE values before IDENTITY bucket hashing. Exact filters serialize legacy DATE as a string, but _assign deserializes both DATE and DATETIME with DatelikeTargetType::DATE_TIME; this branch therefore hashes YYYY-MM-DD 00:00:00, while tablet routing hashes the stored DATE as YYYY-MM-DD. For 2026-01-02 with 3 buckets those encodings select buckets 2 and 1 respectively, so a remote filter can prune the bucket containing the match. Please parse TYPE_DATE as DATE (or normalize by primitive_type) and cover a serialized-filter round trip with a non-power-of-two bucket count.
| if (_hybrid_set->contain_null()) { | ||
| buckets->push_back(RawValue::identity_hash(nullptr, 0, primitive_type, 0, bucket_num)); | ||
| } | ||
| _identity_bucket_prune_hashes.emplace(bucket_num, buckets); |
Contributor
There was a problem hiding this comment.
[P2] Bound the per-bucket-count IDENTITY cache. A scan may select partitions with different bucket counts, and each distinct count retained here stores one uint32_t per exact-set value. At the defaults, bucket counts 1..768 and a 40,960-value filter retain about 120 MiB and execute about 31 million hashes for this one runtime filter, with multiple filters multiplying both costs; the map has no cache-level bound or eviction. Please retain only the deduplicated selected buckets, compute per-count values transiently, or enforce a strict aggregate/LRU limit, and cover many distinct partition bucket counts.
Problem Summary: IDENTITY runtime-filter bucket pruning retained one uint32_t per exact-set value for every selected partition bucket count. A 40,960-value filter over counts 1 through 768 retained about 120 MiB of vector elements even when many values selected the same buckets. Cache only distinct bucket IDs using an allocator-aware temporary set, construct a compact owning vector, and stop hashing once all buckets are selected. Keep the existing mutex, cache sharing, hash semantics and CRC32 path unchanged. The stress test bounds retained vector capacity to 295,296 elements (about 1.13 MiB) for those counts, including a NULL value. Reduce memory consumption of IDENTITY runtime-filter bucket pruning across partitions with different bucket counts, without changing query results.
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28173 ms |
Contributor
TPC-DS: Total hot run time: 151734 ms |
Contributor
ClickBench: Total hot run time: 24.97 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Problem Summary: The latest upstream DATE round-trip test still calls the CRC32-only one-argument bucket-prune hash API. After merging master into the IDENTITY branch, that call fails to compile because the API requires an explicit hash algorithm and bucket count. Adapt the actual test caller instead of adding a compatibility overload or default arguments. Preserve its raw CRC32 assertion and also verify IDENTITY buckets against the original DATE for bucket counts 3, 7 and 97. Production hashing and its three-argument interface remain unchanged.
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28144 ms |
Contributor
TPC-DS: Total hot run time: 152234 ms |
Contributor
ClickBench: Total hot run time: 24.01 s |
Contributor
BE Regression && 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
3 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?
Issue Number: close #66231
Problem Summary:
Hash Bucketing previously supported only the CRC32 hash function to map a row to its bucket. There was no way for users to choose a different bucketing scheme, which prevents scenarios (e.g. bucket pruning / colocation with external systems) that rely on a plain identity mapping of the bucket key, as shown in #66231.
This PR introduces a table-level
distribution_hash_typeproperty for Hash Bucketing tables, supporting two values:crc32(default, keeps existing behavior), meanscrc32(key) % bucket_num.identity(map rows to buckets by the raw key value), meanskey % bucket_num.Main changes:
distribution_hash_typeproperty inPropertyAnalyzer; carry the hash type throughHashDistributionInfo,HashDistributionDesc,ColocateGroupSchema,DataPartition,DistributionSpecHashand related create-table / colocate paths; a newFeMetaVersionis added for metadata persistence.ADD PARTITIONnow inherits the table hash type.HashDistributionPruner,PruneOlapScanTablet) still works correctly for identity buckets.TDistributionHashTypeis added to thrift and threaded through the exchange / local-exchange sink operators and the tablet-sink partitioner so that the backend uses the same bucketing function as the FE plan, keeping FE/BE bucketing consistent. An identity partitioner is implemented.TDistributionHashTypeand propagate the field inDescriptors,Partitions,PlanNodesandTypes.Before this PR: only CRC32 bucketing was available.
After this PR: users can create Hash Bucketing tables with
"distribution_hash_type" = "identity", and FE plan optimization plus BE data sink produce consistent bucketing results. And more hash functions can be extended in the future.Release note
Support a new table property
distribution_hash_typefor Hash Bucketing tables. It acceptscrc32(default) andidentitynow, allowing users to choose the hash function used to map rows to buckets.Check List (For Author)
Test
Behavior changed:
distribution_hash_typeproperty is added; but default behavior (crc32) is unchanged.Does this need documentation?
Check List (For Reviewer who merge this PR)