github-actions[bot] commented on code in PR #67768:
URL: https://github.com/apache/doris/pull/67768#discussion_r3975972172
##########
be/src/exec/operator/hashjoin_probe_operator.cpp:
##########
@@ -438,6 +439,10 @@ Status HashJoinProbeOperatorX::_do_evaluate(Block& block,
VExprContextSPtrs& exp
// TODO: if const-key optimization is added, update
_extract_join_column() together.
block.get_by_position(result_col_id).column =
block.get_by_position(result_col_id).column->convert_to_full_column_if_const();
+ // Probe keys must be normalized the same way as the build keys, see
+ // HashJoinBuildSinkLocalState::_do_evaluate().
+ normalize_float_hash_key(block.get_by_position(result_col_id).column,
Review Comment:
[P1] Preserve the original probe/build row values until all equality keys
plus other and mark conjuncts have run. For a direct `VSlotRef`,
`result_col_id` is the actual input slot, so this replaces the value seen by
later expressions and copied into the candidate output block. For example, with
left `x=-0.0` and right `x=+0.0`, `ON l.x = r.x AND signbit(l.x) <>
signbit(r.x)` should match, but both operands are `+0.0` when
`do_other_join_conjuncts()` executes and the row is dropped. Use a separate
normalized key column/holder, and add this predicate as a regression case.
##########
be/src/core/column/column_vector.cpp:
##########
@@ -232,6 +239,11 @@ uint32_t ColumnVector<T>::_crc32c_hash_value(uint32_t
hash, const value_type& va
const auto& date_val = (const VecDateTimeValue&)value;
auto len = date_val.to_buffer(buf);
return crc32c_extend(hash, (const uint8_t*)buf, len);
+ } else if constexpr (is_float_or_double(T)) {
+ // See _zlib_crc32_hash: equal floating point values must hash alike.
+ auto normalized = value;
+ NormalizeFloat(normalized);
+ return HashUtil::crc32c_fixed(normalized, hash);
Review Comment:
[P1] Gate this hash convention for rolling upgrades. An old BE hashes raw
`-0.0`, while this helper on a new BE hashes canonical `+0.0`; with three
destinations those zlib hashes route to channels 2 and 1 respectively, and
CRC32C changes as well. Because neither partitioner branches on
`be_exec_version`, pinning FE to the old version does not make mixed senders
agree, so equal build/probe rows can reach different join instances. The
Float64 bloom convention is likewise transported without a hash-scheme marker.
Please introduce a new execution/protocol version and retain the legacy
convention for lower versions across shuffle and bloom producer/probe paths.
##########
be/src/core/column/column_vector.cpp:
##########
@@ -220,6 +221,12 @@ uint32_t ColumnVector<T>::_zlib_crc32_hash(uint32_t hash,
size_t idx) const {
const auto& date_val = (const VecDateTimeValue&)data[idx];
auto len = date_val.to_buffer(buf);
return HashUtil::zlib_crc_hash(buf, len, hash);
+ } else if constexpr (is_float_or_double(T)) {
+ // -0.0 == +0.0 and all NaN payloads compare equal, so they must hash
to the same
+ // partition; otherwise equal keys land on different shuffle/spill
partitions.
+ auto value = data[idx];
+ NormalizeFloat(value);
Review Comment:
[P1] Cover the single-value CRC path used by composite columns. This
`_zlib_crc32_hash()` branch fixes top-level
`ColumnVector::update_crcs_with_value()`, but
`ColumnArray`/`ColumnMap`/`ColumnStruct` delegate nested leaves through
`ColumnVector::update_crc_with_value_without_null()`, which still hashes raw
float bytes. With `enable_new_shuffle_hash_method=false`, `array(-0.0)` and
`array(+0.0)` can still be sent to different receivers before join-local
normalization. Refactor nested CRC32 traversal through the canonical helper and
add a forced legacy-shuffle nested-key test.
##########
be/src/exec/common/hash_table/hash.h:
##########
@@ -159,6 +161,21 @@ inline size_t hash_crc32(doris::Int128 u) {
return doris::UInt128HashCRC32()({(u >> 64) & int64_t(-1), u &
int64_t(-1)});
}
+// Doris equality treats -0.0 == +0.0 and every NaN payload as the same value,
so such keys
+// must hash alike (bloom filters and hash tables built through HashCRC32
would miss them
+// otherwise). Ordinary values keep the exact hash of their integer bit
pattern.
+template <>
+inline size_t hash_crc32(doris::Float32 u) {
Review Comment:
[P1] Route `TYPE_FLOAT` bloom filters through a defined canonical hash.
`fixed_len_to_uint32_v2` takes its `sizeof(T) <= 4` branch for `Float32` and
executes `(uint32_t)value`, so this specialization is never used; converting
NaN (or an out-of-range float) to `uint32_t` is undefined. The added
`HashFloatTest` therefore does not exercise the producer/probe path, and the
FLOAT regression only covers signed zero. Handle Float32 before the size branch
and add a real `BloomFilterFunc<TYPE_FLOAT>` test with distinct NaN payloads.
--
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]
