zghong commented on PR #66477: URL: https://github.com/apache/doris/pull/66477#issuecomment-6064448437
@HappenLee Thanks for your thorough reviews and follow-up. Here is a brief summary of each item at the current commit `0ffddf6`: > [P1] Preserve the probe-side IDENTITY layout through broadcast joins Fixed in `6d9bcebcb9`. Broadcast hash joins now inherit the probe-side layout. Unit tests and broadcast-to-bucket-join regressions cover both local-shuffle planner settings. > [P1] Normalize storage hash metadata when converting to EXECUTION_BUCKETED Fixed in `0422bfc2ab`. Distribution-spec constructors normalize execution-shuffle hash metadata, including properties converted from IDENTITY storage layouts. The merge assertion remains to reject genuinely incompatible storage layouts. > [P1] Handle TIMESTAMP_NS in IDENTITY bucketing before accepting it as a distribution column Fixed in `a34b91eb04`. It now uses the eight-byte IDENTITY encoding, with targeted BE hashing and FE pruning tests. The test-coverage is consistent with the `zlib crc32` algorithm. > [P1] Preserve the probe-side IDENTITY layout through Nested Loop Join Fixed in `eaf9760024`. NLJ layout propagation now follows the probe side, consistent with optimizer properties. Unit tests and NLJ-to-bucket-join regressions cover both planner settings. > [P1] Reject IDENTITY plans when the configured execution version is below 15 Fixed in `bc338dd47f`. IDENTITY metadata serialization now checks the configured minimum execution version across write and query paths. The current minimum is 16, following upstream version allocation. > [P1] Do not materialize this cache on a wrapper that can still be merged Not changed in this PR. The sharing/publication path and cache-before-merge invariant already exist in the baseline, including the CRC32 path. We agree with the follow-up that this should be tracked separately as a pre-existing lifecycle risk; production reachability still needs a controlled reproducer. > [P1] Normalize remote DATE values before IDENTITY bucket hashing Fixed upstream in `cd25f2cf99`, which is already included in the current baseline. Serialized-filter round-trip tests also cover IDENTITY hashing. > [P2] Bound the per-bucket-count IDENTITY cache Fixed in `65bb8d3077` by retaining only deduplicated bucket IDs and stopping once all buckets are covered, with many-bucket-count tests, and we chose the deduplication alternative suggested in the review. > [P2] Keep the fragment layout check within the fragment and respect broadcast join output distribution Fixed in `d242f0c3bb`. Collection stops at Exchange boundaries, ignores non-bucket Exchange default tags, and follows probe-side output semantics for broadcast joins and NLJs. SQL-to-Thrift tests cover both local-shuffle settings, while incompatible layouts remain rejected. What's more: 1) issues that were discovered during self-reviewing and testing, but were not introduced by this PR, have already been submitted separately in #68749 and #68750. 2) the relevant documentation has also been updated. Finally, thanks again for your valuable comments. If there are any further questions, I am glad to keep the branch up to date and address any additional feedback. -- 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]
