fix(freshcore): count duplicate rows natively for detection-only dedup - #405
Merged
Merged
Conversation
With drop_duplicates=False the native module never counted duplicates, so engine="freshcore" recorded no detection, no duplicate_threshold warning, and duplicate_ratio_action="error" never raised natively. The native module now counts full-row duplicates at the pandas dedup stage (after string cleaning, empty-row removal and casts; before imputation and outliers), returns it as duplicates_detected and records a detect_duplicates stage timing. The duplicates kernel shares one duplicated_mask between counting and dropping, and, like pandas, finds no duplicates in a frame without columns. The adapter keeps its pandas fallback under duplicate_ratio_action="error" for native modules built before this change, which do not report the count. Closes #323
Adds a freshcore-native job: installs a pinned Rust toolchain with rustup, runs the crate's cargo tests, builds freshdata_freshcore with maturin develop and runs the FreshCore tests against it. No other job installs the extension, so the native parity tests skip everywhere else.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|
Authored-code reduction (Metric 6)
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
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.
Summary
With
drop_duplicates=False(the default), the FreshCore native module never counted duplicate rows. Soengine="freshcore"recorded no detection and noduplicate_thresholdwarning, andduplicate_ratio_action="error"raised only through a pandas fallback. #375 already fixed the adapter side; this PR does the native part.crates/freshcore: the duplicates kernel now has oneduplicated_mask, shared by a newcount_duplicatesand bydrop_duplicates. As in pandas, a frame with no rows or no columns has no duplicates.execute_plan: counts duplicates at the same stage as the pandas dedup step (after string cleaning, empty-row removal and casts; before imputation and outliers), returnsduplicates_detected, and records adetect_duplicatesstage timing.duplicates_detectedthe way pandas does. It keeps its pandas fallback underduplicate_ratio_action="error"for native modules built before this change, which don't report the count.freshcore-nativejob runscargo test, builds the extension with maturin, and runs the FreshCore tests against it. It uses a pinned Rust toolchain (1.98.1), SHA-pinned actions and a 10-minute timeout. No other job installs the extension, so the native parity tests are skipped everywhere else.docs/freshcore.md(new "Building and testing" section),docs/fallback-matrix.md,docs/backends.md, CHANGELOG.Behaviour change:
engine="freshcore"with detection-only dedup now reports detected duplicates, warns above the threshold and raisesDuplicateRatioErrornatively under"error", all matching pandas.Tests
count_duplicates, forduplicated_maskunder first/last, that the count equals the rows dropped, and for frames without rows or columns.tests/test_execution/test_freshcore_native_parity.py(skipped withoutfreshdata_freshcore). Native runs usefallback_policy="error", so any fallback fails the test.DuplicateRatioErrornatively under"error"and matches pandas under"warn".string_case, empty rows and numeric casts create duplicates.test_missing_native_module_falls_back_to_pandasnow simulates the missing module, so it passes where the extension is built.Verification
cargo test --locked: 12 passed.cargo clippy: no warnings in the changed code.maturin develop: the parity file passed (28 tests), andpytest tests/test_execution -k freshcorepassed (98 tests, 1 skipped for pyspark).ruff check .andmypy src/freshdata: clean.Closes #323