mrhhsg commented on code in PR #67962:
URL: https://github.com/apache/doris/pull/67962#discussion_r4256748934
##########
be/src/core/column/column_struct.cpp:
##########
@@ -259,15 +292,34 @@ void ColumnStruct::update_crcs_with_value(uint32_t*
__restrict hash, PrimitiveTy
void ColumnStruct::update_crc32c_batch(uint32_t* __restrict hashes,
const uint8_t* __restrict null_map)
const {
- for (const auto& column : columns) {
- column->update_crc32c_batch(hashes, nullptr);
+ auto s = size();
+ if (null_map) {
Review Comment:
Re-evaluated this at `1ed9b82` against the code. Keeping the PR as the
semantic fix without a `be_exec_version` fence, for these reasons:
1. **The remote-exchange exposure is bounded to the CRC32C path, which
already has an operator-level switch.** `exchange_sink_operator.cpp` /
`local_exchange_sink_operator.cpp` only pick `Crc32CHashPartitioner` (the sole
caller of `update_crc32c_batch` on the shuffle path) when the session variable
`enable_new_shuffle_hash_method` is true. With it false the sink uses
`Crc32HashPartitioner` -> `update_crcs_with_value`, and
`ColumnStruct::update_crcs_with_value` already skips outer-NULL rows on both
old and new BEs (untouched by this PR). An operator who wants byte-identical
routing during the rolling window can `SET GLOBAL
enable_new_shuffle_hash_method = false` for the window. That is the same
query-level lever #59052 relied on when it introduced the CRC32C partitioner
itself: old BEs ignore the thrift flag and keep legacy CRC while new BEs use
CRC32C, and that change was not fenced by `be_exec_version` either.
2. **`be_exec_version` cannot fence this fix where it will actually be
deployed.** `be_exec_version_manager.cpp` states that new versions may only be
introduced in X.Y.0 releases, must not be forked in past releases, and that the
field should not be used for compatibility fixes. This is a bug fix that will
go to the release branches, where no new version can be added, so a fence on
master would leave every patch-release rolling upgrade exactly as exposed as
today. On master it would also mean threading the version through every
`IColumn` hash virtual (`update_crc32c_batch/single`,
`update_xxHash_with_value`, `update_crc_with_value`,
`update_hashes_with_value`) and the `ColumnNullable` / `ColumnArray` /
`ColumnMap` recursion, keeping the known-wrong hidden-payload hash reachable
under the old version.
3. **FE does not negotiate the version per query.**
`CoordinatorContext.java:310` and `Coordinator.java:396` send the static
`Config.be_exec_version`; nothing lowers it to the minimum across the executing
BEs, so a fence only changes behaviour in the window between the BE and FE
upgrades.
4. **What is exposed, concretely.** During the rolling window, a `GROUP BY`
on a nullable STRUCT key (or an ARRAY/MAP containing one) across a *remote*
exchange with sender instances on both old and new BEs can split the NULL key
into two groups, and only when the NULL rows carry identical hidden payloads
(stored NULLs). Whenever payloads differ (the IF/CASE reproducer here) old BEs
already split those keys today, even in a homogeneous cluster, which is the bug
this PR fixes. The same holds for the xxHash consumers (`multi_distinct` over
ARRAY keys): wrong today when payloads differ, transiently divergent for
identical payloads during the window. Nothing is persisted from these hashes,
so the effect ends with the upgrade.
Resolving on that basis. If a maintainer still wants the version fence, it
should be a separate follow-up that adds the plumbing and a new version number
per the manager's policy.
--
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]
