[Exec](set) use bitset container to speed up hash set#62532
[Exec](set) use bitset container to speed up hash set#62532HappenLee wants to merge 1 commit intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Found 1 blocking issue.
Critical checkpoints:
- Goal of the task: Speed up tinyint/smallint IN-set lookups. The current change does not fully accomplish that goal, because it regresses the non-constant
INexecution path forSMALLINT. - Small, clear, focused change: The patch is small, but the unconditional type-based switch is too broad because it overrides the existing small-
Nspecialization everywhere. - Concurrency: No new concurrency or locking concerns were introduced in the reviewed code paths.
- Lifecycle / static initialization: No special lifecycle or static initialization issues were found.
- Configuration: No new configs.
- Compatibility: No storage / protocol compatibility concerns were found.
- Parallel code paths: Yes, there are parallel callers of
create_set(). In particular,FunctionIn::impl_without_set()is negatively affected by this change. - Special conditional checks: The new
TYPE_TINYINT || TYPE_SMALLINTbranch lacks a size threshold, so it bypasses the existingFixedContainerfast path even for tiny RHS sets. - Test coverage: Existing
be/test/exprs/hybrid_set_test.cppstill covers basic membership, but there is no new test covering the changed container-selection behavior or the non-constantSMALLINT IN (...)path that regresses here. - Observability: No additional observability seems necessary.
- Transaction / persistence / data writes / FE-BE variable passing: Not applicable to this PR.
- Performance: Blocking regression.
FunctionIn::impl_without_set()builds a temporary set per input row viacreate_set(arg_type, set_datas.size(), true). After this patch, everySMALLINTrow now allocates and zero-fills a 65,536-entry container even when the RHS has only 1-8 elements, replacing the previous fixed-container fast path with ~64 KiB of work per row. - Other issues: None beyond the blocker above.
Please preserve the existing fixed-container path for small N, and only switch to BitSetContainer once the set is large enough to amortize its initialization cost.
| static BasePtr get_function(bool null_aware) { | ||
| if constexpr (is_string_type(type)) { | ||
| return new StringSet<>(null_aware); | ||
| } else if constexpr (type == TYPE_TINYINT || type == TYPE_SMALLINT) { |
There was a problem hiding this comment.
create_set() is also used by FunctionIn::impl_without_set() (be/src/exprs/function/in.h) for non-constant RHS expressions, and that path builds a temporary set per input row. With this branch ahead of the existing N <= FIXED_CONTAINER_MAX_SIZE check, every SMALLINT fallback set now becomes a BitSetContainer<int16_t> even when the RHS only has 1-8 elements. That means each row pays for allocating and zero-filling a 65,536-slot container instead of using the previous fixed-container fast path. For queries like smallint_col IN (other_smallint1, other_smallint2, ...), this turns the fallback into ~64 KiB of work per row and can easily dominate execution time. Please keep the small-N FixedContainer path and only choose BitSetContainer once the cardinality is large enough to amortize its initialization cost.`
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
What problem does this PR solve?
use bitset container to speed up hash set