Skip to content

FIX - keep aux_table column order for default cols in AggJoiner and MultiAggJoiner - #2250

Merged
rcap107 merged 1 commit into
skrub-data:mainfrom
dylanpulver:fix-aggjoiner-default-cols-order
Aug 24, 2026
Merged

FIX - keep aux_table column order for default cols in AggJoiner and MultiAggJoiner#2250
rcap107 merged 1 commit into
skrub-data:mainfrom
dylanpulver:fix-aggjoiner-default-cols-order

Conversation

@dylanpulver

Copy link
Copy Markdown
Contributor

Bug Fix Pull Request

Description

AggJoiner and MultiAggJoiner derive their default cols from a set difference, so the aggregated columns come out in a different order on each interpreter run. No existing issue; repro on main at 9fb3ac5:

import pandas as pd
from skrub import AggJoiner

aux = pd.DataFrame({
    "userId": [1, 1, 2], "rating": [4.0, 3.0, 5.0], "year": [1999, 2000, 2001],
    "budget": [10.0, 20.0, 30.0], "votes": [7.0, 8.0, 9.0],
    "runtime": [90.0, 100.0, 110.0], "screens": [1.0, 2.0, 3.0],
})
aj = AggJoiner(aux, key="userId", operations=["mean"])
print(aj.fit(pd.DataFrame({"userId": [1, 2]}))._cols)
PYTHONHASHSEED=0  ['runtime', 'rating', 'screens', 'votes', 'year', 'budget']
PYTHONHASHSEED=1  ['year', 'screens', 'runtime', 'votes', 'budget', 'rating']
PYTHONHASHSEED=2  ['runtime', 'budget', 'screens', 'votes', 'year', 'rating']

The constraint on the replacement is that the default path has to agree with the explicit one. When cols is passed, _cols is the caller's list in the caller's order, so aux table order is the only choice that makes cols=None equivalent to passing every non-key column by hand.

That is why sorted() was rejected. It is deterministic, but it would give the default path an alphabetical order the explicit path never produces, and the two forms of the same request would disagree.

(~s.cols(*key)).expand(table) is the selector expression aggregate() uses in _agg_joiner.py for this same "everything but the key" selection. Selector.expand iterates sbd.column_names, which pins the result to the table's own order.

Behavior change worth naming: for cols=None the output column order moves for any run that happened to land on a different permutation. Per-column values are unchanged, and no order was reproducible before, so nothing could have depended on a particular one.

Residual: aggregate() also builds num_only_op from a set intersection, so the operations listed in that AttributeError are unordered too. That is message text rather than output, and it is untouched here.

Checklist

  • I have read the contributing guidelines
  • I have added tests that verify the bug fix
  • I have added an entry to CHANGES.rst describing the fix
  • My code follows the code style of this project
  • I have checked my code and corrected any misspellings

How Has This Been Tested?

Full suite on this branch against a clean checkout of main at 9fb3ac5: 3029 passed vs 3026, with the same single pre-existing failure in both (skrub/datasets/tests/test_fetching.py::test_warning_redownload_checksum_has_changed, which downloads).

Reverting the two source hunks while keeping the tests: the new test_agg_joiner_default_cols_order fails on every PYTHONHASHSEED from 0 to 11, across all three df_module variants. The two pre-existing assertions that called sorted() were tightened to exact order; those are weaker detectors, catching the old behavior on 10 of 30 and 9 of 12 seeds respectively, because permutations of two or three columns often coincide with table order by chance.

ruff 0.14.4, the version pinned in .pre-commit-config.yaml, reports no check or format changes.

AI Disclosure

  • This PR contains AI-generated code
    • I have tested the code generated in my PR
    • I have read and understood every line that has been generated by the AI agent
    • I can explain what the AI-generated code does

@rcap107 rcap107 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot @dylanpulver ! Simple and useful fix

…ultiAggJoiner

When `cols` is not provided, the columns to aggregate were collected with a
set difference, so their order varied between interpreter runs and the
aggregated output columns came out in a different order each time.

Use the `(~s.cols(*key)).expand(table)` selector idiom that `aggregate()`
uses in the same module, which walks `sbd.column_names` and therefore keeps
the auxiliary table's own column order.
@dylanpulver
dylanpulver force-pushed the fix-aggjoiner-default-cols-order branch from a93eab8 to 527d7ba Compare August 24, 2026 18:45
@rcap107
rcap107 merged commit abf0147 into skrub-data:main Aug 24, 2026
29 checks passed
rcap107 pushed a commit to rcap107/skrub that referenced this pull request Aug 27, 2026
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.

2 participants