Fix null-safe result joins - #1356
Merged
mwojtyczka merged 10 commits intoJul 28, 2026
Merged
Conversation
mattfaltyn
marked this pull request as ready for review
July 21, 2026 17:58
8 tasks
mwojtyczka
added
the
under-review
mwojtyczka
self-requested a review
July 23, 2026 10:17
mwojtyczka
reviewed
Jul 23, 2026
mwojtyczka
left a comment
mwojtyczka
left a comment
Contributor
There was a problem hiding this comment.
Review — null-safe result joins
Solid, correct fix for the null-key silent-drop bug (#1354). I verified the four rewired joins are row-count-equivalent to the old left-joins for non-null keys (every result_df is a groupBy().agg() — one row per key) and now additionally match null keys, and ran the new regressions locally (3 passed). No blocking issues.
The notes below are robustness/hardening observations on the new shared helper and one pre-existing naming gap it now routes through — none block merge. Inline comments follow.
mwojtyczka
requested changes
Jul 23, 2026
Contributor
Author
|
Thank you @mwojtyczka for your review. All comments have been addressed in commit 2abf5c2. |
Spell out the three caller responsibilities the helper relies on but does not validate: non-empty join_columns (an empty list reduces the null-safe join to a cross join), one row per join key in result_df (else df rows fan out), and result_columns disjoint from df.columns (else duplicate output columns). All current callers satisfy these; the note is for future reuse. Co-authored-by: Isaac
mwojtyczka
approved these changes
Jul 28, 2026
mwojtyczka
left a comment
mwojtyczka
left a comment
Contributor
There was a problem hiding this comment.
LGTM - adjusted docstring
mwojtyczka
added
Approved to Merge
Two integration tests still encoded the pre-null-safe join behavior: - test_apply_checks_with_sql_query: the multiple_key_check_violation check merges on [b, c]. With the old left join, rows whose b was NULL could not match their own query group (NULL == NULL is NULL, not True), so no violation attached. The null-safe join now matches them (NULL <=> NULL), so rows (2, NULL, 3) and (1, NULL, 4) correctly gain the violation. - test_aggr_matches_dataset_count_distinct_group_by_null_key and the aggr_matches_dataset docstring claimed count_distinct's grouped join is not null-safe. Both the checked-side two-stage groupBy join and the reference-side join are in fact null-safe, so a NULL group key is compared like any other. Updated the test to assert the NULL group matches and passes, and corrected the docstring. Verified both tests pass against a live workspace. Co-authored-by: Isaac
1 task
This was referenced Jul 29, 2026
mwojtyczka
added a commit
that referenced
this pull request
Jul 30, 2026
## Changes - Reattach `has_no_gaps_per_time_window` results with the existing null-safe result-join helper. - Preserve gap violations for groups with null key components without changing the public API or non-null behavior. - Add a focused Spark integration regression for a null-key group with an interior gap. ### Linked issues Fixes #1386 Related: #1354, #1356 ### Tests - [x] manually tested — focused regression passed with stock PySpark 4.0.0 - [ ] added unit tests — the affected behavior requires Spark - [x] added integration tests - [ ] added end-to-end tests — no workflow or external-resource behavior changed - [ ] added performance tests — the change reuses the existing result-join helper at one call site Validation performed: - `make fmt` - `make lint` - `make test` — 1,549 passed - Isolated stock-PySpark 4.0.0 smoke reproduction — null-key boundary row flagged, following row passed The workspace-backed focused integration test cannot run locally without `DATABRICKS_HOST`; upstream integration CI provides the authoritative run. ### Documentation and Demos - [ ] added/updated demos — not applicable - [ ] added/updated docs — the fix restores existing documented behavior without changing the API - [ ] added/updated agent skills — no public API or usage contract changed --------- Co-authored-by: Marcin Wojtyczka <marcin.wojtyczka@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
count_distinctgroups, and nullable grouped aggregate-outlier violations.Spark equality joins do not match
NULLkeys, so these paths could compute a real violation and then silently discard it while joining the result back to the input. The shared helper aliases both sides, uses null-safe equality for every key component, and selects only the original left-side columns plus the computed result columns.Linked issues
Fixes #1354
Tests
Validation performed:
make fmtmake lintcount_distinct, and grouped aggregate outliersDocumentation and Demos