Fix duplicate-key row multiplication in dataset comparisons - #1488
Fix duplicate-key row multiplication in dataset comparisons#1488mattfaltyn wants to merge 3 commits into
Conversation
44ccc7c to
abeb08f
Compare
ghanse
left a comment
There was a problem hiding this comment.
Left a comment here regarding design: #1487 (comment)
Let me know what you think.
Thanks @ghanse. Implemented in 41d9bc9. Strict uniqueness remains the default; |
ghanse
left a comment
There was a problem hiding this comment.
Solid PR overall — it matches the design we landed on in #1487 (deterministic pairing for duplicate keys), with good test and doc coverage, and I didn't spot any correctness bugs.
My one real design question is whether the default should raise at all: raising needs an eager action, whereas the row_number() pairing you added is already lazy and already fixes the fan-out — so it might make a better default, with the hard-fail as an opt-in. Not a hard blocker, but worth a discussion. The rest are minor.
| ): | ||
| matchable_rows = dataset if null_safe_row_matching else dataset.dropna(subset=matching_columns) | ||
| duplicate_keys = matchable_rows.groupBy(*matching_columns).agg(F.count("*").alias(match_count_col)) | ||
| if not duplicate_keys.where(F.col(match_count_col) > 1).isEmpty(): |
There was a problem hiding this comment.
The bigger thing: this default path fires two eager Spark jobs (.isEmpty() on each side) on every comparison, even when there are no dupes — but compare_datasets is otherwise lazy like the rest of our checks. The catch is you can't raise without an action, so any "validate then raise" is inherently eager.
Worth flipping this around: the row_number() pairing you added below is already fully lazy and already prevents the fan-out that #1487 is about. If that's the default, the bug is fixed with zero extra jobs, and unique keys are unchanged (rn=1 on both sides, so joining on (pk, rn) is the same as joining on pk). If we still want to surface non-unique keys, we can do it lazily instead of raising:
duplicate_key_condition = F.count(F.lit(1)).over(Window.partitionBy(*pk_column_names)) > 1and fold it into the status JSON like row_missing/changed. A hard raise-and-stop is still reasonable, but since it's inherently eager I'd make it the opt-in flag (raise_on_duplicate_keys=True) rather than the default. Curious what you think — happy to be talked out of it if the raise needs to be the default.
There was a problem hiding this comment.
If that's the default, the bug is fixed with zero extra jobs
Agreed, thanks! Pairing is now lazy by default; raise_on_duplicate_keys=True opts into eager validation. Added regressions for laziness and null-key pairing.
| def apply(df: DataFrame, spark: SparkSession, ref_dfs: dict[str, DataFrame]) -> DataFrame: | ||
| ref_df = _get_ref_df(ref_df_name, ref_table, ref_dfs, spark) | ||
|
|
||
| if df.isStreaming or ref_df.isStreaming: |
There was a problem hiding this comment.
Good guard — the window/groupBy machinery wouldn't behave on streaming anyway, so a clear error beats a cryptic one. Just flagging it's technically a new restriction for any streaming caller, so worth a line in the changelog/release notes.
There was a problem hiding this comment.
worth a line in the changelog/release notes.
Added the batch-only restriction to the breaking-change notes, with guidance for streaming callers. Thanks for flagging it!
| ref_join_columns = ref_pk_column_names | ||
| if allow_duplicate_keys: | ||
| row_number_col = f"__match_row_number_{unique_id}" | ||
| order_columns = [F.col(col).cast("string").asc_nulls_first() for col in compare_columns] or [F.lit(1)] |
There was a problem hiding this comment.
Minor: casting everything to string means numerics order lexically ("10" < "2"). Doesn't affect correctness — the diff runs on the real values and identical rows still cluster together — just noting the pairing order is a bit arbitrary for numbers. Fine to leave as-is.
There was a problem hiding this comment.
Fine to leave as-is.
Thanks! Kept the ordering and documented lexical numeric sorting; comparisons still use the original values.
| refs_df = {"ref_df": make_ref_df} | ||
| checked = dq_engine.apply_checks(generated_df, checks, refs_df) | ||
| actual_count = benchmark(lambda: checked.count()) | ||
| actual_count = benchmark.pedantic( |
There was a problem hiding this comment.
pedantic(rounds=1, iterations=1) is a single measurement with no repeats, so this won't catch perf regressions anymore — it's basically a smoke test now. Was that intentional? If you want a real signal, keeping the default multi-round benchmark() on at least the strict case would help.
There was a problem hiding this comment.
keeping the default multi-round
benchmark()on at least thestrictcase would help.
Good catch! Restored multi-round benchmark() for both modes, including check construction so strict validation is measured.
Changes
compare_datasetswith lazy one-to-one pairing by default. Rows within each matching-key group receive arow_number()ordered by the string representation of compared values, with nulls first.raise_on_duplicate_keys=Trueto opt into eager uniqueness validation on both datasets. With standard null matching, rows containing null matching keys are excluded from uniqueness validation because they cannot match.This prevents many-to-many joins from multiplying output rows and reporting false changes between identical datasets, while keeping the default comparison lazy.
Linked issues
Resolves #1487
Tests
Local validation:
make fmtmake lintmake test— 2,484 passedUV_FROZEN=1 UV_BUILD_CONSTRAINT=.build-constraints.txt uv run --exact --all-extras --group docs pydoc-markdown— succeeded with three cross-reference warnings in unchanged aggregate helperscorepack yarn --cwd docs/dqx build— 150 documents indexed, production build succeededThe repository's workspace-backed integration fixture could not initialize because
DATABRICKS_HOSTis unset. The local Spark checks do not replace Databricks Connect, Unity Catalog, or workspace-scale benchmark validation.Documentation and Demos