Adds ignore_order for groupBy agg test that returns multiple rows [databricks]#14044
Conversation
Signed-off-by: Alessandro Bellina <abellina@nvidia.com>
|
build |
Greptile SummaryThis PR fixes a test flakiness issue by adding the Key changes:
The fix is minimal, correct, and follows established conventions in the codebase. The actual aggregation logic and computed values (avg=63.0) are correct on both GPU and CPU - only the ordering differed. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as test_avg_divide_by_zero
participant Spark as Spark Engine
participant GPU as GPU Executor
participant CPU as CPU Executor
Test->>Spark: Create DataFrame (id % 2 as k, id as v)
Test->>Spark: Execute groupBy("k").agg(avg(...))
par GPU Execution
Spark->>GPU: Execute query
GPU->>GPU: Group by k (0, 1)
GPU->>GPU: Compute avg(CASE WHEN k>0...)
Note over GPU: Returns rows in order:<br/>[k=1, avg=63.0]<br/>[k=0, avg=None]
GPU-->>Test: GPU Result
and CPU Execution
Spark->>CPU: Execute query
CPU->>CPU: Group by k (0, 1)
CPU->>CPU: Compute avg(CASE WHEN k>0...)
Note over CPU: Returns rows in order:<br/>[k=0, avg=None]<br/>[k=1, avg=63.0]
CPU-->>Test: CPU Result
end
Test->>Test: Compare results with @ignore_order
Note over Test: Order differences ignored<br/>Test passes ✓
|
sameerz
left a comment
There was a problem hiding this comment.
Would be good to know the root cause of the ordering change in our underlying systems. Approving given the change is in tests only, and the ordering should not matter.
|
Possibly related: an upgrade to CCCL 3.2 NVIDIA/spark-rapids-jni#4094 |
I've bisected the cudf commits and can confirm that this is due to rapidsai/cudf#20796, which changes the behavior of hash-partitioning. |
Fixes #14043
Description
The test test_avg_divide_by_zero performs a groupBy("k") which returns multiple rows (k=0 and k=1), but it does NOT use @ignore_order, which means that we've been getting lucky in the ordering. That said #14043 documents a failure where spark 3.3.0 and Databricks is returning a different order. Since the order of the keys isn't deterministic, I added an
@ignore_order.The unit test was added here #13192
Checklists
(Please explain in the PR description how the new code paths are tested, such as names of the new/existing tests that cover them.)