Skip to content

fix: return selector union columns in schema order - #4032

Open
SAY-5 wants to merge 1 commit into
narwhals-dev:mainfrom
SAY-5:fix-selector-union-order
Open

SAY-5 wants to merge 1 commit into
narwhals-dev:mainfrom
SAY-5:fix-selector-union-order

Conversation

@SAY-5

@SAY-5 SAY-5 commented Oct 5, 2026 •

Copy link
Copy Markdown

Description

CompliantSelector.__or__ put the left selector's columns first, so ncs.boolean() | ncs.numeric() came back as ['d', 'a', 'c'] on pandas, pyarrow and duckdb while Polars gives schema order. The union now filters df.columns by the selected names. test_set_ops now checks the exact order instead of sorted(...), with a case where the left selector's column comes after the right one's.

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

AI assistance

  • No AI tools were used for this PR.
  • AI tools were used.

Checklist

  • Code follows style guide (ruff)

  • Tests added

  • Documented the changes

  • If this is your first PR to narwhals, attach a screenshot of pytest passing locally (not CI):

    PYTEST_ADDOPTS="--numprocesses=logical" \
    make run-ci DEPS="--extra pandas --extra dask --group core-tests --group sklearn --group plugins" \
    CMD="pytest tests --cov=src --cov=tests --runslow --constructors=pandas,pandas[nullable],pandas[pyarrow],pyarrow,polars[eager],polars[lazy],dask,duckdb,sqlframe"

Summary by CodeRabbit

  • Bug Fixes
    • Selector unions now preserve dataframe column order and consistently use the right-hand selector’s series when both selectors include the same column.

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
Generated by CodeRabbit — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d0b4e75b-1439-44bf-92e9-6f363c23dfa2
📥 Commits

Reviewing files that changed from the base of the PR and between 1acc082 and e09ac72.

📒 Files selected for processing (2)
  • src/narwhals/_compliant/selectors.py
  • tests/selectors_test.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Selector unions now return selected series and names in dataframe column order. When both selectors select the same column, the right selector’s series is used. Tests check the expected ordering.

Changes

Selector union ordering

Layer / File(s) Summary
Order selector union results
src/narwhals/_compliant/selectors.py, tests/selectors_test.py
Union results and names follow dataframe column order. The test checks that boolean() | numeric() returns ["a", "c", "d"] without sorting the result.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: fbruzzesi

Merge Risk: ⚪ Minimal · up to e09ac

Selector unions now return columns in dataframe order, and the regression test checks that behavior. No supported-input merge blocker remains.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to e09ac

Selector unions now follow dataframe column order while preserving right-side precedence for overlapping selections. The inspected change introduces no material security risk or new privileged operation.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected delta affects output ordering within the dataframe supplied to a selector invocation. It does not introduce access to another tenant, asset, datastore, credential, or execution environment.

Trust Boundaries and Controls

  • observed — The same two selector callables are evaluated before and after the change. The added column-name filtering orders already-selected results without introducing an identity transition, additional authority, or a new privileged sink.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: selector unions now return columns in dataframe schema order.
Description check ✅ Passed The description explains the bug, the fix, and the ordering test. It also identifies the PR type and related issues. The AI-assistance choice is unchecked, but this does not make the description incom…
Linked Issues check ✅ Passed Issue #4029 requires selector unions to return selected columns in dataframe schema order and tests for reversed selector order. CompliantSelector.__or__ now orders both selected series and names by…
Out of Scope Changes check ✅ Passed The reviewed changes address issue #4029. The source change updates selector-union ordering, and the test change verifies that behavior. No unrelated changes are shown in the PR summary or the reviewe…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
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.

bug: Selector union (|) returns columns in a different order on non-Polars backends

1 participant