mrhhsg commented on code in PR #67768:
URL: https://github.com/apache/doris/pull/67768#discussion_r3978732055
##########
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:
Addressed in 831b67f: the column-level hash changes are gone.
`Crc32HashPartitioner::do_partitioning()` (exchange, local exchange, spill /
spill re-partition) and the bloom runtime filter (`fixed_len_to_uint32_v3`,
selected through `RuntimeFilterParams::normalize_float_keys`) now normalize
float keys only when the query runs with `be_exec_version >= 15`
(`NORMALIZE_FLOAT_HASH_KEY_VERSION`, FE `max_be_exec_version` raised
accordingly). Every BE of a query receives the same version from FE, so pinning
FE to 14 during a rolling upgrade keeps the legacy raw-bit convention on all
senders and on both bloom producer and consumer.
`PartitionerFloatKeyTest.DoubleKeyIsNormalizedOnlyFromTheNewVersion` and
`BloomFilterFuncTest.FindFixedLenDoubleSignedZero` pin both versions.
##########
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:
Addressed in 831b67f: keys are no longer normalized in place.
`_extract_join_column()` hashes a normalized copy of a float key column (probe
side: `_key_columns_holder` per batch; build side:
`HashJoinSharedState::normalized_build_key_columns`, which the hash table
points into for the probe phase), while the block itself keeps its shape and
stored values, so other join conjuncts, mark conjuncts and the output are
unaffected. The same applies to INTERSECT / EXCEPT (`build_col_idx` keeps the
original column) and to partition TopN. Regression
`order_qt_other_conjunct_on_key_sign` joins on `o.x = i.b and cast(o.x as
string) <> cast(i.b as string)` and returns exactly the two pairs whose zeros
differ in sign; `HashJoinProbeOperatorTest.InnerJoinFloatSpecialValueKeys` and
`ExceptOperatorTest.test_float_signed_zero_value_preserved` check the bit
patterns of the output.
##########
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:
Addressed in 831b67f: `ColumnVector::_zlib_crc32_hash()` is back to the base
version. The partitioner now hashes a normalized copy of the whole key column
(`replace_float_special_values()` recurses through array / map / struct /
nullable leaves), so the legacy zlib path
(`enable_new_shuffle_hash_method=false`), crc32c and the spill partitioners all
agree for nested keys. Covered by
`PartitionerFloatKeyTest.NestedFloatKeyIsNormalized` and the regression blocks
`legacy_shuffle_array` (forced legacy shuffle with `array(o.x) = array(i.b)`)
and `spill_join_array` (`enable_force_spill`).
##########
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:
Addressed in 831b67f: the `HashCRC32<Float32/Float64>` specializations are
removed. Bloom filters use a new `fixed_len_to_uint32_v3` that handles FLOAT
and DOUBLE before the `sizeof(T) <= 4` branch (NormalizeFloat, then the CRC32
of the bit pattern), selected per query by
`RuntimeFilterParams::normalize_float_keys` and carried by `light_copy()` to
the storage-level bloom predicate; the legacy v2 conversion is kept for older
query versions. `BloomFilterFuncTest.FindFixedLenFloatSpecialValues` builds a
`BloomFilterFunc<TYPE_FLOAT>` with `+0.0` / quiet NaN and probes `-0.0`, a
payload NaN and `-NaN` through `find_fixed_len_olap_engine`, `find_fixed_len`,
`find_batch_raw_fixed` and `test_field`; the regression
`bloom_runtime_filter_float` block covers the FLOAT filter end to end.
--
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]
