mrhhsg commented on code in PR #67962:
URL: https://github.com/apache/doris/pull/67962#discussion_r4037025501
##########
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:
Good catch, fixed in d1098a38fbb. The masked batch paths
(`update_crc32c_batch` and `update_hashes_with_value`) no longer route
surviving rows through the single/range hooks. They now hash every row through
each field's batch hook exactly as the unmasked path does, then restore the
outer-NULL rows from a saved copy of the input hashes. Surviving rows therefore
keep the child batch contract (e.g. the 8-byte NULL default of a nullable
BIGINT), and the dispatch count stays at one call per field regardless of how
many outer NULLs the block has.
The masked range paths (`update_crc32c_single`, `update_xxHash_with_value`,
`update_crc_with_value`) now walk maximal non-NULL runs column by column, so an
all-zero mask is the single run `[start, end)` and yields the same hashes and
the same number of calls as the unmasked path.
Added
`ColumnStructTest.BatchHashKeepsChildBatchContractWithUnrelatedOuterNull`
(nullable BIGINT field, nonzero seeds, block with vs. without an unrelated
outer NULL, both crc32c and xxHash batch) and
`ColumnStructTest.RangeHashWithAllZeroMaskMatchesUnmasked`. Since the masked
paths now make the same number of virtual calls as the unmasked ones, I did not
add a benchmark.
##########
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:
I don't think a `be_exec_version` fence is the right tool here, and I'd
rather not add one:
- The old behavior is not a stable semantic to preserve: it hashes an
outer-NULL struct by whatever hidden payload happens to sit under the NULL
flag, so two old BEs already disagree with each other for the reproducer in
this PR. Keeping that behavior reachable under an older exec version would keep
the wrong-result path alive.
- `be_exec_version_manager.cpp` states that new versions may only be
introduced in X.Y.0 releases and warns against using the field for this kind of
fix; bumping it for a column-level hash bug fix would also require threading
the version into every column hash virtual.
- FE sends the static `Config.be_exec_version` (default
`max_be_exec_version`) and does not lower it to the minimum across BEs, so the
fence would only take effect when operators lower the config by hand during the
upgrade.
- Previous changes to the column hash entry points (#59052, #64944) did not
fence either.
The exposure is limited to the rolling-upgrade window and to shuffles keyed
on a nullable STRUCT (or ARRAY/MAP containing one) with NULL values, which
already return wrong results today. If the maintainers still want a fence I can
add one, but I would prefer to keep this PR as the semantic fix.
##########
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:
Done in d1098a38fbb: removed `def tableName` and hardcoded
`agg_groupby_nullable_struct_ls` in all 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]
