-
Notifications
You must be signed in to change notification settings - Fork 1k
[FEA] Improve join_match_context API design and usability #20958
Copy link
Copy link
Open
Labels
feature requestNew feature or requestNew feature or requestlibcudfAffects libcudf (C++/CUDA) code.Affects libcudf (C++/CUDA) code.
Milestone
Description
Is your feature request related to a problem? Please describe.
The join_match_context and join_partition_context APIs have design decisions that were intentionally made but have proven suboptimal:
- Non-standard naming: Public members use leading underscores (
_left_table,_match_counts) which is conventionally reserved for private members - Redundant unique_ptr wrapper:
std::unique_ptr<rmm::device_uvector<size_type>>adds dereferencing overhead but provides no benefit sincedevice_uvectoris already non-copyable and move-only - Inconsistent terminology:
hash_joinusesprobe/build(implementation-specific) while storing as_left_table. Should useleft_table/right_table(deterministic, matches join result semantics) - No encapsulation: Direct member access prevents future changes without breaking users
- Code duplication:
left_join_match_contextandfull_join_match_contexthave identical implementations
Describe the solution you'd like
- Remove leading underscores from public members
- Store container directly:
struct join_match_context { table_view left_table; rmm::device_uvector<size_type> match_counts; // No unique_ptr wrapper };
- Add accessor methods (if converting to class):
device_span<size_type const> match_counts_span() const; table_view left_table() const;
- Standardize on
left_table/right_tableterminology across all join types
Describe alternatives you've considered
- Minimal approach: Add accessor methods while keeping struct members public (backward compatible)
- Hybrid approach: Convert
join_match_contextto class but keepjoin_partition_contextas struct - Keep current design: Maintain backward compatibility at the cost of usability improvements
Additional context
Current usage requires awkward dereferencing:
auto match_context = hash_join.inner_join_match_context(probe, stream);
expect_match_counts_equal(*match_context._match_counts, {1, 0, 2, 1, 2}, stream); // Must dereference
// Proposed: cleaner direct access with same ownership semantics
expect_match_counts_equal(match_context.match_counts, {1, 0, 2, 1, 2}, stream);The partition API works well for chunking large datasets (documented in sort_merge_join.hpp). A potential enhancement would be adding a helper for equal-sized chunks:
// Proposed helper for common chunking pattern
static std::vector<join_partition_context> equal_chunks(
join_match_context const& ctx, size_type chunk_size);
// Would simplify current manual loop:
for (size_type start = 0; start < num_rows; start += chunk_size) {
size_type end = std::min(start + chunk_size, num_rows);
auto part_ctx = join_partition_context{context, start, end};
// ...
}Related APIs: sort_merge_join::inner_join_match_context, hash_join::{inner,left,full}_join_match_context
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
feature requestNew feature or requestNew feature or requestlibcudfAffects libcudf (C++/CUDA) code.Affects libcudf (C++/CUDA) code.