Fix assertion failures in assert_tpch_result_equal due to float sort ambiguity

[Matt711]

Description

When comparing results, sorting by non-float columns alone can leave rows with equal non-float keys in an arbitrary order, causing assert_frame_equal to fail on valid results. This PR retries the comparison using float columns as a secondary sort key before raising a validation error.

• Closes [BUG] Validate TPC-DS Q64 #22129

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[TomAugspurger]

So my original thinking here was that sorting on the float columns should be unnecessary. Suppose you have a sequence of (key, value) pairs, sorted by key:

Key, Value
0, x
0, x-e
0, x+e

Assuming e (or more precisely, 2e) is negligible, then any permutation of those rows should be considered equal to any other. And so my hope was that we could sort by Key and then validate with

1. assert_frame_equal on Key without any abs_tol
2. assert_frame_equal on all the columns with abs_tol

we'd correctly implement that logic. It would pass as long as abs_tol >= 2e, and fail otherwise (on the second stage).

[TomAugspurger]

I retract all my concerns about this change :) This was a bug in how we handled the float columns.

At the time of the assertion error, we've already validated that the sort_by columns match. Now we're just dealing with non-sort_by columns.

This stage works by

1. Sorting on all non-float columns.
2. Doing an assert_frame_equal on all columns with a tolerance.

So IIUC, the issue is when you have a pair of tables that are equal on all non-float columns, but for some
reason the float columns are in a different order (but have equal values). For example, table 1:

A B
1 1.0
1 2.0
1 3.0

and table 2:

A B
1 2.0
1 3.0
1 1.0

We want these two to compare equal, but they currently don't because the float columns.

The simplest fix seems to be to sort by all columns, but in a specific order: non-float first, then float. I've done that in 65827dc, along with a test that was previously failing.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Tests
  • Enhanced test assertion logic to more accurately compare results by improving sort-order handling for floating-point values and tied rows.
  • Expanded test coverage with new validation cases for result comparison edge cases.

Walkthrough

The assert_tpch_result_equal function now sorts columns by type (non-float first, float last) via sort_for_comparison and applies this grouped sort across all comparison branches (sort_by non-ties, sort_by ties, and no sort_by). New tests cover permuted float rows within non-float groups and out-of-tolerance failures.

Changes

Grouped float column sorting for result comparison

Layer / File(s)	Summary
Sort-for-comparison helper definition python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py	Splits columns into non-float and float groups and defines sort_for_comparison helper that sorts both groups in sequence, with float columns placed last.
Precompute grouped-sorted frames when sort_by provided python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py	When sort_by is provided, computes left_sorted and right_sorted using sort_for_comparison for later equality checks.
Apply grouped sorting to .sort_by(...).head(n) non-ties comparison python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py	In the sort_by-driven .head(n) non-ties logic, applies sort_for_comparison to result_first and expected_first before equality checks instead of sorting only by non-float columns.
Apply grouped sorting to .sort_by(...).head(n) ties comparison python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py	In the ties branch, applies sort_for_comparison to result_ties.select(by) and expected_ties.select(by) before equality checks.
Apply grouped sorting to unsorted frames python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py	When sort_by is not provided, applies grouped sorting (non-float then float) to both frames before asserting equality, replacing direct frame comparison to ignore nondeterministic row ordering.
Test grouped float sorting behavior python/cudf_polars/tests/testing/test_asserts.py	Adds tests: one ensuring payload column permutation does not affect order when sorting by tie keys, and a parametrized test verifying grouped float-sort equality within tolerance and failure beyond tolerance.

---

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	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 accurately describes the main change: fixing assertion failures in assert_tpch_result_equal due to float column sorting ambiguity.
Description check	✅ Passed	The description clearly explains the problem (sorting by non-float columns leaves rows in arbitrary order) and the solution (retry using float columns as secondary sort keys).
Linked Issues check	✅ Passed	The PR successfully addresses issue #22129 by implementing deterministic sorting that handles tied non-float keys using float columns as secondary sort keys, making validation robust to ordering ambiguity.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the float sort ambiguity issue: the assertion logic is updated and comprehensive test coverage is added for both tie-order and grouped-float sort scenarios.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py (1)

375-394: 💤 Low value

Consider extracting common sorting logic to reduce duplication.

The column classification (non_float_columns, float_columns, grouped_sort_columns) and sorting logic is duplicated between the if sort_by: branch (lines 263-278) and this else branch. Extracting the helper function and column lists before the if sort_by: block would reduce maintenance burden.

Proposed refactor

Move the column classification and helper before the if sort_by: block:

     left = left.with_columns(*float_casts)
 
+    non_float_columns = [
+        col
+        for col in left.columns
+        if left.schema[col] not in (pl.Float32, pl.Float64)
+    ]
+    float_columns = [
+        col for col in left.columns if left.schema[col] in (pl.Float32, pl.Float64)
+    ]
+    grouped_sort_columns = [*non_float_columns, *float_columns]
+
+    def sort_for_comparison(df: pl.DataFrame) -> pl.DataFrame:
+        return (
+            df.sort(by=grouped_sort_columns, nulls_last=nulls_last)
+            if grouped_sort_columns
+            else df
+        )
+
     if sort_by:
         by, descending = list(zip(*sort_by, strict=True))
         # ... sortedness checks ...
-        non_float_columns = [
-            col
-            for col in left.columns
-            if left.schema[col] not in (pl.Float32, pl.Float64)
-        ]
-        float_columns = [...]
-        grouped_sort_columns = [...]
-        def sort_for_comparison(df: pl.DataFrame) -> pl.DataFrame:
-            ...
         left_sorted = sort_for_comparison(left)
         right_sorted = sort_for_comparison(right)
         # ...
     else:
-        non_float_columns = [...]
-        float_columns = [...]
-        grouped_sort_columns = [...]
-        left_sorted = (
-            left.sort(by=grouped_sort_columns, nulls_last=nulls_last)
-            if grouped_sort_columns
-            else left
-        )
-        right_sorted = (...)
+        left_sorted = sort_for_comparison(left)
+        right_sorted = sort_for_comparison(right)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py` around
lines 375 - 394, Extract the duplicated column-classification and sorting logic
into a small helper and run it once before the if sort_by: branch: compute
non_float_columns (columns where schema not in pl.Float32/Float64),
float_columns (columns where schema in pl.Float32/Float64), and
grouped_sort_columns (concatenate the two), then create a helper function (e.g.,
sort_by_grouped_columns(left, right, grouped_sort_columns, nulls_last)) that
returns left_sorted and right_sorted using left.sort(...) and right.sort(...)
when grouped_sort_columns is non-empty; call this helper from both the existing
if sort_by: branch and the else branch so the column lists and sorting code are
not duplicated.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py`:
- Around line 375-394: Extract the duplicated column-classification and sorting
logic into a small helper and run it once before the if sort_by: branch: compute
non_float_columns (columns where schema not in pl.Float32/Float64),
float_columns (columns where schema in pl.Float32/Float64), and
grouped_sort_columns (concatenate the two), then create a helper function (e.g.,
sort_by_grouped_columns(left, right, grouped_sort_columns, nulls_last)) that
returns left_sorted and right_sorted using left.sort(...) and right.sort(...)
when grouped_sort_columns is non-empty; call this helper from both the existing
if sort_by: branch and the else branch so the column lists and sorting code are
not duplicated.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 19b7a51b-8bbf-4c4b-ae8c-c26003a6422e

📥 Commits

Reviewing files that changed from the base of the PR and between d09d10d and 65827dc.

📒 Files selected for processing (2)

• python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py
• python/cudf_polars/tests/testing/test_asserts.py

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py`:
- Around line 218-233: The helper sort_for_comparison currently closes over the
full original grouped_sort_columns which lets payload columns influence
ordering; change it to derive the actual sort keys from the frame being compared
by inspecting df.schema/df.columns (e.g. build a local_grouped list containing
only the grouped_sort_columns present in df), then call
df.sort(by=local_grouped, nulls_last=nulls_last) (or return df when
local_grouped is empty) so tie partitions are aligned using only the shared
sort_by keys (this makes comparisons like
sort_for_comparison(result_ties.select(by)) vs
sort_for_comparison(expected_ties.select(by)) stable).
- Around line 369-376: The current normalization via sort_for_comparison masks
row-order differences even when check_row_order is True; change the logic so
canonicalization only happens when not check_row_order: keep original left/right
when check_row_order is True and only call sort_for_comparison for the not
check_row_order path, then call polars.testing.assert_frame_equal on the
appropriately chosen left/right and preserve the check_row_order flag (or
explicitly set check_row_order=False only in the canonicalized branch with a
comment explaining order is intentionally ignored). Reference symbols:
sort_for_comparison, left_sorted, right_sorted,
polars.testing.assert_frame_equal, and the check_row_order parameter.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c961925a-22b8-4cc7-8069-a9d604ec2507

📥 Commits

Reviewing files that changed from the base of the PR and between 9c66981 and 75a5e60.

📒 Files selected for processing (1)

• python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[Matt711]

/ok to test eca8958

[Matt711]

/ok to test 29e75ec

[coderabbitai]

🧹 Nitpick comments (1)

python/cudf_polars/tests/testing/test_asserts.py (1)

509-538: ⚡ Quick win

Add edge-case variants for grouped-float sort validation.

Please extend this parametrized test path with at least empty, all-null, and single-element frames to lock down the new ordering logic across boundary cases.

As per coding guidelines, "Missing edge case coverage in tests - Include tests for empty, all-null, single-element, and mixed type cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/cudf_polars/tests/testing/test_asserts.py` around lines 509 - 538,
Extend the parametrized test test_assert_tpch_result_equal_grouped_float_sort to
include edge-case DataFrame variants (empty frame, all-null values in the float
column, and single-element frames) so the grouped-float ordering logic is
exercised at boundaries: add new cases that construct left/right (and
right_different) as empty polars DataFrames, frames where column "c" is all
None/NaN, and frames with a single row, then ensure the same drop_columns and
sort_by adjustments are applied and the same assertions (both success and
expected ValidationError) are executed for each variant to validate behavior
across those edge cases.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@python/cudf_polars/tests/testing/test_asserts.py`:
- Around line 509-538: Extend the parametrized test
test_assert_tpch_result_equal_grouped_float_sort to include edge-case DataFrame
variants (empty frame, all-null values in the float column, and single-element
frames) so the grouped-float ordering logic is exercised at boundaries: add new
cases that construct left/right (and right_different) as empty polars
DataFrames, frames where column "c" is all None/NaN, and frames with a single
row, then ensure the same drop_columns and sort_by adjustments are applied and
the same assertions (both success and expected ValidationError) are executed for
each variant to validate behavior across those edge cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: baf331d5-7b67-4ad3-a9e2-a95eab322529

📥 Commits

Reviewing files that changed from the base of the PR and between 75a5e60 and 29e75ec.

📒 Files selected for processing (2)

• python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py
• python/cudf_polars/tests/testing/test_asserts.py

🚧 Files skipped from review as they are similar to previous changes (1)

• python/cudf_polars/cudf_polars/experimental/benchmarks/asserts.py