github-actions[bot] commented on code in PR #67962:
URL: https://github.com/apache/doris/pull/67962#discussion_r4006242224
##########
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:
[P1] Fence the new struct hash semantics across rolling upgrades
This changes the actual shuffle hash for every outer-NULL struct: an old BE
appends the hidden field payload after the nullable marker, while a new BE
stops at the marker. In a mixed-version remote exchange, the same logical key
can therefore go to different receivers (with the production seed and three
channels, hidden `Int32(0)` maps to channel 2 on the parent and channel 1
here), splitting `GROUP BY`/join state. The partitioner calls this API without
any `be_exec_version` branch, and `enable_new_shuffle_hash_method` is already
default true on both versions. Please make this a query-versioned semantic
choice (or otherwise prevent mixed-version producers from sharing the affected
exchange), apply the fence to the changed recursive hash paths, and cover
old/new hash vectors.
##########
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) {
+ for (size_t i = 0; i < s; ++i) {
+ if (null_map[i] == 0) {
+ update_crc32c_single(i, i + 1, hashes[i], nullptr);
Review Comment:
[P1] Preserve crc32c batch semantics for surviving rows
When any outer row is NULL, this sends every surviving struct through
`update_crc32c_single()`, but that is not equivalent to the child batch
contract. For example, a NULL `Nullable(BIGINT)` field is hashed by
`ColumnNullable::update_crc32c_batch()` as the nested eight-byte default, while
its single hook hashes a fixed four-byte `int 0`. Starting from the production
shuffle seed those map to channels 2 and 1 of 3, so the same non-NULL
`STRUCT<x:NULL>` can be routed differently solely because another row in its
block is outer-NULL. The new Int32 fixture misses this because both encodings
are four bytes and its hashes start at zero. Please preserve each child's batch
semantics while suppressing parent-NULL rows (for example with a two-mask or
scratch-result path), and add a nonzero-seed nullable-BIGINT vector comparing
blocks with and without an unrelated outer NULL. The batched design should also
avoid the dispatch regression here: under the normal 8,160-row FE batch, one
outer NUL
L in a 100-field struct makes 815,900 child virtual calls instead of 100;
please cover the sparse-NULL and all-zero nested-range cases with a benchmark
or focused performance test.
##########
regression-test/suites/query_p0/aggregate/aggregate_groupby_nullable_struct_local_shuffle.groovy:
##########
@@ -0,0 +1,73 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("aggregate_groupby_nullable_struct_local_shuffle") {
+ // IF(cond, NULL, named_struct(...)) yields logical NULL structs whose
hidden field payloads
+ // differ per row. GROUP BY must treat them as one group no matter how the
rows are hashed
+ // by the local shuffle before aggregation.
+ def tableName = "agg_groupby_nullable_struct_ls"
Review Comment:
[P3] Hardcode the ordinary single-table test name
Please follow the regression-test convention for an ordinary single-table
suite: remove `def tableName` and hardcode `agg_groupby_nullable_struct_ls` in
the DROP/CREATE/INSERT/query SQL.
--
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]
