Skip to content

Fix drop_duplicates() for non-trivial Indexes#925

Merged
ehsantn merged 4 commits into
mainfrom
ehsan/fix_drop_dups_with_index
Nov 13, 2025
Merged

Fix drop_duplicates() for non-trivial Indexes#925
ehsantn merged 4 commits into
mainfrom
ehsan/fix_drop_dups_with_index

Conversation

@ehsantn

@ehsantn ehsantn commented Nov 12, 2025

Copy link
Copy Markdown
Collaborator

Changes included in this PR

Also fixes dropna flag in drop_duplicates' groupby call. Fixes semi join test of Narwhals.

Testing strategy

Added unit test.

User facing changes

Bug fix.

Checklist

  • Pipelines passed before requesting review. To run CI you must include [run CI] in your commit message.
  • I am familiar with the Contributing Guide
  • I have installed + ran pre-commit hooks.

@ehsantn ehsantn mentioned this pull request Nov 12, 2025
10 tasks
@codecov

codecov Bot commented Nov 12, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.79%. Comparing base (c33fbb5) to head (45f50ac).
⚠️ Report is 119 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #925      +/-   ##
==========================================
+ Coverage   66.68%   68.79%   +2.10%     
==========================================
  Files         186      195       +9     
  Lines       66795    67566     +771     
  Branches     9507     9595      +88     
==========================================
+ Hits        44543    46481    +1938     
+ Misses      19572    18253    -1319     
- Partials     2680     2832     +152     

@ehsantn

ehsantn commented Nov 12, 2025

Copy link
Copy Markdown
Collaborator Author

The CI failure seems flakiness (coverage file not found.)

@scott-routledge2 scott-routledge2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ehsantn

}
)
df.loc[99, "B"] = np.nan
# TODO: Add groupby dropna flag to DuckDB's aggregate node so the flag is

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue for this yet?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue and added the issue number.

@DrTodd13 DrTodd13 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. lgtm

@ehsantn ehsantn merged commit 35cec95 into main Nov 13, 2025
29 checks passed
@ehsantn ehsantn deleted the ehsan/fix_drop_dups_with_index branch November 13, 2025 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants