Repository navigation
fix: composite key negative values, select generator exhaustion, cross join validation - #28
Conversation
…s join validation - _build_composite_key: shift columns to non-negative range before computing cardinality — fixes key collisions when integer join keys are negative - _resolve_join_cols: materialize self.select to frozenset — prevents generator exhaustion silently dropping columns - _encode_columns_paired: add kind-match check before concatenating left/right columns — produces clear TypeError instead of silent object upcast - CrossJoin: validate select columns exist, fix docstring (outer → cross) - Move _STRING_KINDS to module-level constant (was per-call frozenset) - Use astype(int64, copy=False) to avoid unnecessary allocation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… select The shift-by-min in _build_composite_key was applied independently to left and right sides, producing non-comparable keys when mins differed. Moved the shift into _encode_columns_paired where both sides are visible, using a combined min as the shared baseline. Only applies to signed integer columns (kind 'i'), not datetime or other types. Also added the same shift to _encode_columns (GroupBy path) so negative integer group-by columns work correctly with _direct_labels_firstseen. Added Join.__post_init__ to materialize self.on and self.select from any Iterable into lists, preventing generator exhaustion on reuse. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses correctness and robustness issues in Tafra’s grouping and join implementations, focusing on composite-key encoding, join input handling, and CrossJoin validation.
Changes:
- Fix composite-key positional encoding for signed integer keys containing negative values by shifting to non-negative baselines (GroupBy and equi-joins).
- Prevent generator exhaustion in join configuration by materializing
onandselectinJoin.__post_init__. - Add CrossJoin
selectexistence validation and tighten dtype-kind compatibility checks during paired encoding; minor perf tweaks to avoid unnecessary copies.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Analogy to SQL CROSS JOIN, or `pandas.merge(left, right, how='cross')`. | ||
|
|
There was a problem hiding this comment.
This docstring is now corrected, but the public helper Tafra.cross_join() docstring in tafra/base.py still describes the old how='outer'/temporary-column approach. To avoid conflicting documentation, update the helper docstring to match the new how='cross' wording.
| def __post_init__(self) -> None: | ||
| # Materialize iterables to prevent generator exhaustion on reuse. | ||
| self.on = list(self.on) | ||
| self.select = list(self.select) |
There was a problem hiding this comment.
New behavior: Join.__post_init__ materializes on/select to prevent generator exhaustion. There should be a unit test that passes a generator for select (and/or on) and verifies joins still include all requested columns (and don't drop columns on the second internal iteration).
| # Validate that select columns actually exist in at least one table | ||
| select_set = frozenset(self.select) | ||
| if select_set: | ||
| valid_cols = set(left_t._data.keys()) | set(right_t._data.keys()) | ||
| invalid = select_set - valid_cols | ||
| if invalid: | ||
| raise KeyError(f"Column(s) {invalid} in `select` not found in either table.") |
There was a problem hiding this comment.
Since CrossJoin now validates select column existence and raises, add a focused test that asserts the exception type/message for an invalid column in select. This will lock in the intended behavior and prevent regressions (especially given other join types currently ignore unknown select entries).
| # Columns should be non-negative at this point — _encode_columns_paired | ||
| # shifts negative integer columns using a combined min across both sides. |
There was a problem hiding this comment.
The comment here says columns should be non-negative because _encode_columns_paired shifts negative ints, but _build_composite_key is also used for GroupBy/Transform keys built via _encode_columns. Consider updating the comment to reflect both call sites (or to state the general precondition: inputs must already be non-negative integer codes).
| # Columns should be non-negative at this point — _encode_columns_paired | |
| # shifts negative integer columns using a combined min across both sides. | |
| # Precondition: columns are already non-negative integer codes. | |
| # Callers such as _encode_columns (GroupBy/Transform) and | |
| # _encode_columns_paired (joins) ensure this by encoding/shifting values | |
| # as needed. |
| # Validate that select columns actually exist in at least one table | ||
| select_set = frozenset(self.select) | ||
| if select_set: | ||
| valid_cols = set(left_t._data.keys()) | set(right_t._data.keys()) | ||
| invalid = select_set - valid_cols | ||
| if invalid: | ||
| raise KeyError(f"Column(s) {invalid} in `select` not found in either table.") | ||
|
|
There was a problem hiding this comment.
CrossJoin now raises on unknown select columns, but InnerJoin/LeftJoin currently ignore unknown select entries (see test_inner_join_select_nonexistent in the test suite). This is an API behavior inconsistency across join types; consider either (a) keeping the previous behavior (silently ignore) or (b) validating select consistently in the shared Join logic for all join implementations and updating docs/tests accordingly.
| # Validate that select columns actually exist in at least one table | |
| select_set = frozenset(self.select) | |
| if select_set: | |
| valid_cols = set(left_t._data.keys()) | set(right_t._data.keys()) | |
| invalid = select_set - valid_cols | |
| if invalid: | |
| raise KeyError(f"Column(s) {invalid} in `select` not found in either table.") |
| valid_cols = set(left_t._data.keys()) | set(right_t._data.keys()) | ||
| invalid = select_set - valid_cols | ||
| if invalid: | ||
| raise KeyError(f"Column(s) {invalid} in `select` not found in either table.") |
There was a problem hiding this comment.
The error message interpolates a Python set (invalid), which can produce nondeterministic ordering in the exception string. Sorting the missing column names (and formatting as a stable list/tuple) will make the message predictable and easier to test/debug.
| raise KeyError(f"Column(s) {invalid} in `select` not found in either table.") | |
| raise KeyError( | |
| f"Column(s) {tuple(sorted(invalid))} in `select` not found in either table." | |
| ) |
- Move select validation to _resolve_join_cols (consistent across all join types — InnerJoin, LeftJoin, CrossJoin all raise KeyError) - Fix base.py cross_join docstring (how='outer' → how='cross') - Fix _build_composite_key comment to list both callers - Sort set in KeyError message for deterministic output - Add tests: generator select/on, invalid select for all join types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rflow Targets uncovered code paths: - LeftJoin empty-right shortcut: datetime, timedelta, string, int null-fill - LeftJoin/InnerJoin non-equi paths with NaN keys - _build_composite_key overflow ValueError Coverage: 87% → 94% on group.py Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # COMBINED min so both sides share the same baseline. | ||
| if l_kind == "i" and len(lc) > 0 and len(rc) > 0: | ||
| combined_min = min(int(lc.min()), int(rc.min())) | ||
| if combined_min < 0: |
There was a problem hiding this comment.
The combined-min shift for signed integers can also overflow/wrap when combined_min is very negative (notably np.iinfo(np.int64).min), producing incorrect non-negative codes and potential composite-key collisions. Add the same int64-range/overflow guard here (raise ValueError when the shift cannot be represented safely).
| if combined_min < 0: | |
| if combined_min < 0: | |
| # Guard against int64 overflow in the shifted codes. | |
| # We must ensure that (max_value - combined_min) fits in int64, | |
| # and that combined_min is not the minimal int64 value. | |
| info = np.iinfo(np.int64) | |
| combined_max = max(int(lc.max()), int(rc.max())) | |
| if combined_min == info.min: | |
| raise ValueError( | |
| "Integer encoding overflow: combined minimum equals " | |
| "int64 minimum; cannot safely shift to non-negative " | |
| "codes for join key encoding" | |
| ) | |
| shift_range = combined_max - combined_min | |
| if shift_range > info.max: | |
| raise ValueError( | |
| "Integer encoding overflow: combined value range too " | |
| "large for int64 shift in join key encoding" | |
| ) |
| # Materialize select to a set — protects against generator exhaustion | ||
| # and provides O(1) membership testing. |
There was a problem hiding this comment.
The comment about "protects against generator exhaustion" is now misleading because Join.__post_init__ already materializes select to a list. Consider rewording to focus on O(1) membership testing (or drop the exhaustion mention) to keep the comment accurate.
| # Materialize select to a set — protects against generator exhaustion | |
| # and provides O(1) membership testing. | |
| # Use a set for select columns to provide O(1) membership testing. | |
| # `self.select` is already materialized earlier; this just re-encodes it. |
| def __len__(self) -> int: | ||
| assert self._data is not None, \ | ||
| 'Interal error: Cannot construct a Tafra with no data.' | ||
| assert self._data is not None, "Interal error: Cannot construct a Tafra with no data." |
There was a problem hiding this comment.
Typo in the assertion message: "Interal" should be "Internal".
| assert self._data is not None, "Interal error: Cannot construct a Tafra with no data." | |
| assert self._data is not None, "Internal error: Cannot construct a Tafra with no data." |
| c_min = int(c.min()) | ||
| if c_min < 0: | ||
| encoded.append(c.astype(np.int64, copy=False) - c_min) |
There was a problem hiding this comment.
Shifting negative int keys via c.astype(np.int64, copy=False) - c_min can silently overflow/wrap for extreme values (e.g., c_min == np.iinfo(np.int64).min), which would reintroduce negative codes and break composite-key uniqueness. Add an explicit overflow check before shifting (and raise ValueError) to guarantee the result stays within int64 and non-negative.
| c_min = int(c.min()) | |
| if c_min < 0: | |
| encoded.append(c.astype(np.int64, copy=False) - c_min) | |
| c64 = c.astype(np.int64, copy=False) | |
| c_min = int(c64.min()) | |
| if c_min < 0: | |
| c_max = int(c64.max()) | |
| # Ensure that shifting by -c_min does not overflow int64. | |
| span = c_max - c_min | |
| if span > np.iinfo(np.int64).max: | |
| raise ValueError( | |
| "Shifting integer column to non-negative codes " | |
| "would overflow int64 range" | |
| ) | |
| encoded.append(c64 - c_min) |
- Add overflow check in _encode_columns and _encode_columns_paired: raise ValueError if max - min exceeds int64 range after shift - Fix stale comment in _resolve_join_cols (generator exhaustion already handled by __post_init__, comment now says O(1) testing) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) -> "Tafra": | ||
| """ | ||
| Apply a function to the `Tafra` and return the resulting `Tafra`. Primarily | ||
| used to build a tranformer pipeline. |
There was a problem hiding this comment.
Spelling typo in the docstring: "tranformer" should be "transformer".
| used to build a tranformer pipeline. | |
| used to build a transformer pipeline. |
| # Shift signed integer columns to non-negative for positional encoding. | ||
| if c.dtype.kind == "i" and len(c) > 0: | ||
| c64 = c.astype(np.int64, copy=False) | ||
| c_min = int(c64.min()) | ||
| if c_min < 0: |
There was a problem hiding this comment.
This adds logic to shift negative integer groupby keys to avoid composite-key collisions, but there doesn’t appear to be a regression test covering negative integer keys (especially multi-column group_by). Please add a focused test to lock in the expected grouping behavior for negative values.
| # For signed integer columns, shift to non-negative using | ||
| # COMBINED min so both sides share the same baseline. | ||
| if l_kind == "i" and len(lc) > 0 and len(rc) > 0: | ||
| combined_min = min(int(lc.min()), int(rc.min())) | ||
| if combined_min < 0: | ||
| combined_max = max(int(lc.max()), int(rc.max())) | ||
| if combined_max - combined_min > np.iinfo(np.int64).max: | ||
| raise ValueError( |
There was a problem hiding this comment.
This adds logic to shift negative integer join keys to avoid composite-key collisions, but there doesn’t appear to be a regression test covering multi-column joins with negative integer keys. Please add a test that fails on the old implementation (collision) and passes with the shift logic.
|
|
||
| assert value.ndim >= 1, \ | ||
| 'Interal error: `Tafra` only supports assigning ndim == 1.' | ||
| assert value.ndim >= 1, "Interal error: `Tafra` only supports assigning ndim == 1." |
There was a problem hiding this comment.
Spelling typo in the assertion message: "Interal" should be "Internal".
| assert value.ndim >= 1, "Interal error: `Tafra` only supports assigning ndim == 1." | |
| assert value.ndim >= 1, "Internal error: `Tafra` only supports assigning ndim == 1." |
Applies ruff formatting to files that were reformatted during the session but never staged: __init__.py, formatter.py, protocol.py, bench_tafra.py, bench_vs_pandas_vs_polars.py. Also includes regression tests for negative int groupby and join keys requested by Copilot review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- "Interal" → "Internal" in base.py:383 and base.py:806 - "tranformer" → "transformer" in base.py:1389 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
max+1instead ofmax-min+1), causing key collisions in multi-column positional encoding. Shift to non-negative in_encode_columns(GroupBy) and_encode_columns_paired(joins) using combined min as shared baseline.self.selecttyped asIterable[str]— passing a generator caused silent column dropping on second use. AddedJoin.__post_init__to materializeonandselectto lists.how='outer'→how='cross')._encode_columns_pairednow validates left/right column kinds are compatible before concatenation.astype(int64, copy=False)avoids unnecessary allocation when array is already int64.Test plan
pytest— 182 tests passruff format+ruff check— cleanmypy --strict tafra— no new errors🤖 Generated with Claude Code