mrhhsg commented on code in PR #67962:
URL: https://github.com/apache/doris/pull/67962#discussion_r4257468911
##########
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-checked the two supporting instances from the latest review round against
`1ed9b82`. Both are reachable in the code, so correcting two statements in my
previous reply:
- **Nested nullable `STRUCT` under the legacy CRC path.**
`ColumnStruct::update_crcs_with_value` hashes each non-NULL row via
`update_crc_with_value(i, i + 1, ...)`, and
`ColumnNullable::update_crc_with_value` (`column_nullable.cpp:140-152`) hands
its real null map to the nested column. The same holds for `ColumnArray` /
`ColumnMap` elements. So `enable_new_shuffle_hash_method = false` only keeps
old and new BEs byte-identical for a *top-level* nullable `STRUCT` key; an
inner `Nullable(STRUCT)` with an identical hidden payload still hashes
differently on old/new BEs during the rolling window.
- **`multi_distinct_count` over `ARRAY`.**
`AggregateFunctionUniqExactData<TYPE_ARRAY>` keys the set by the raw `UInt64`
from `update_xxHash_with_value` and `serialize` / `deserialize_and_merge` write
those identifiers verbatim (`aggregate_function_uniq.h:235-256`).
`MultiDistinctCount` does not implement `NotSupportAggState`, so an
`agg_state<multi_distinct_count(ARRAY<...STRUCT...>)>` column can persist old
identifiers. "Nothing is persisted" was too broad for that column type.
This does not change the disposition, because the proposed `be_exec_version`
fence fixes neither instance:
1. A persisted `agg_state` carries no exec version. Under a fence the old
identifiers would keep being produced until the version is raised, and at that
moment the merge of old-identifier states with new identifiers happens exactly
as it would with this PR. The only way to keep those states stable is to keep
hashing the hidden payload, i.e. keep the bug. The over-count is also bounded
and only affects `ARRAY` values that contain a NULL `STRUCT` element: an old
state already holds one identifier per distinct hidden payload (the overcount
this PR fixes), and the upgrade adds at most one more identifier for that value.
2. For the remote-exchange case the earlier policy points stand:
`be_exec_version_manager.cpp` forbids new versions outside X.Y.0 and on release
branches, where this fix must land, and FE sends the static
`Config.be_exec_version` without negotiating the minimum across BEs. The
divergence is confined to NULL `STRUCT` values whose hidden payloads are
identical on old BEs, and it ends with the upgrade; whenever the payloads
differ, old BEs already split the key in a homogeneous cluster today.
What I did instead: the PR description now has a release note and `Behavior
changed: Yes` that spell out both effects (rolling-window routing divergence,
the limited reach of the legacy-hash switch, and the persisted
`multi_distinct_count` state over `ARRAY` of `STRUCT`), so operators can plan
for them. No code change; keeping this thread resolved. If a maintainer wants a
fence despite the above, 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]
