NVIDIA / NVIDIA/cudf

[FEA] Improve join_match_context API design and usability

Open
#20,958 0 comments 0 reactions 0 assignees View on GitHub
feature request libcudf
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

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:

1. **Non-standard naming**: Public members use leading underscores (`_left_table`, `_match_counts`) which is conventionally reserved for private members
2. **Redundant unique_ptr wrapper**: `std::unique_ptr>` adds dereferencing overhead but provides no benefit since `device_uvector` is already non-copyable and move-only
3. **Inconsistent terminology**: `hash_join` uses `probe`/`build` (implementation-specific) while storing as `_left_table`. Should use `left_table`/`right_table` (deterministic, matches join result semantics)
4. **No encapsulation**: Direct member access prevents future changes without breaking users
5. **Code duplication**: `left_join_match_context` and `full_join_match_context` have identical implementations

**Describe the solution you'd like**

1. **Remove leading underscores** from public members
2. **Store container directly**:
```cpp
struct join_match_context {
table_view left_table;
rmm::device_uvector match_counts; // No unique_ptr wrapper
};
```
3. **Add accessor methods** (if converting to class):
```cpp
device_span match_counts_span() const;
table_view left_table() const;
```
4. **Standardize on `left_table`/`right_table`** terminology 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_context` to class but keep `join_partition_context` as struct
- **Keep current design**: Maintain backward compatibility at the cost of usability improvements

**Additional context**

Current usage requires awkward dereferencing:
```cpp
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:
```cpp
// Proposed helper for common chunking pattern
static std::vector 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`

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.