HappenLee commented on code in PR #69296:
URL: https://github.com/apache/doris/pull/69296#discussion_r4348804214
##########
be/src/exprs/vcase_expr.cpp:
##########
@@ -39,17 +43,100 @@ class RuntimeState;
namespace doris {
+namespace {
+
+template <PrimitiveType PType>
+constexpr bool supports_simple_case = is_number(PType) || PType == TYPE_STRING;
+
+template <PrimitiveType PType>
+constexpr size_t simple_case_lookup_threshold() {
+ static_assert(supports_simple_case<PType>);
+ if constexpr (sizeof(typename ColumnElementView<PType>::ElementType) <=
sizeof(uint32_t)) {
+ return 64;
+ } else if constexpr (sizeof(typename
ColumnElementView<PType>::ElementType) <=
+ sizeof(uint64_t)) {
+ return 32;
+ } else {
+ return 8;
+ }
+}
+
+template <PrimitiveType PType>
+struct SimpleCaseLookupState {
+ using Key = typename ColumnElementView<PType>::ElementType;
+ using Hash = std::conditional_t<PType == TYPE_STRING, StringRefHash,
phmap::Hash<Key>>;
Review Comment:
Follow-up on native 128-bit keys: `Int128`/`LARGEINT` and IPv6/UUID also
have a related hash-quality problem on paths using the libstdc++ default hash,
with a different trigger from the Decimal XOR cancellation above.
- `doris::Int128` is native `__int128`; IPv6 and UUID use native `unsigned
__int128`. In the tested x86_64 libstdc++ implementation, their integer
`std::hash` returns `static_cast<size_t>(value)`, discarding the high 64 bits.
`phmap::Hash` inherits that behavior. Distinct keys `(i << 64) | 1` therefore
all hash to `1`; subsequent hash mixing cannot recover the discarded bits.
- An independent probe with the repository's phmap container reproduced this
for both native types: 1024 keys gave one distinct hash, 1,048,576 equality
calls during construction, and 1024 equality calls for one missed lookup. The
same probe using libc++ 16 gave 1024 distinct hashes for these keys. This
standard-library dependency applies to native integers; the explicit Decimal
limb-XOR cancellation remains a separate issue.
- The existing large `IN`/`NOT IN` path is an affected consumer under
libstdc++: `FunctionIn::open` -> `create_set` -> `DynamicContainer` for more
than eight keys -> `DynamicContainerHash` -> `phmap::Hash`. Both LARGEINT and
IPv6 are supported by this factory. This is existing code, not introduced by
this PR.
- Some paths already use whole-value hashing. `ArraySetHash` explicitly
redirects unsigned native 128-bit keys to `DefaultHash`; because IPv6 and UUID
have the same underlying alias, both are covered. The aggregation/join
hash-table aliases inspected also use whole-value CRC hashing. They should not
be classified as affected merely because their keys are 128 bits wide.
`doris::UInt128` (`wide::UInt128`) is also a distinct C++ type from native
`unsigned __int128`, with its own hash implementation.
The current simple-CASE FE eligibility excludes LARGEINT and IPv6, so these
are shared-infrastructure follow-ups rather than additional reachable types for
this PR today. A useful follow-up fix is to select the existing whole-value
Doris hash for native signed/unsigned 128-bit keys in `DynamicContainerHash`,
preserving its float normalization/equality behavior, and validate
fixed-low64/varying-high64 keys on both sides of the small-set threshold.
These results come from source tracing and independent container probes; no
Doris SQL latency benchmark was run.
--
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]
