-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix silent row drops in multi-GPU joins with computed key expressions #22318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7a43ab9
b6731e0
7533aa2
9f73486
49ae7c1
85bf860
1c45fc5
ac05b0f
4063da5
337d4fc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import polars as pl | ||
|
|
||
| from cudf_polars.containers import DataType | ||
| from cudf_polars.dsl import expr | ||
| from cudf_polars.dsl.utils.naming import names_to_indices | ||
|
|
||
|
|
||
| def test_names_to_indices_concrete_prefix() -> None: | ||
| dtype = DataType(pl.Int64()) | ||
| schema = {"a": dtype, "b": dtype, "c": dtype} | ||
| names = ( | ||
| expr.NamedExpr("a_alias", expr.Col(dtype, "a")), | ||
| "b", | ||
| expr.NamedExpr("computed", expr.Literal(dtype, 1)), | ||
| expr.NamedExpr("c_alias", expr.Col(dtype, "c")), | ||
| ) | ||
|
|
||
| assert names_to_indices(names, schema, concrete_prefix=True) == (0, 1) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -388,3 +388,59 @@ def test_dynamic_planning_skips_compile_time_partition_wise_join(): | |
| right_ir: PartitionInfo(1, partitioned_on=()), | ||
| } | ||
| assert not _use_pwise_join(executor, partition_info, join_ir) | ||
|
|
||
|
|
||
| def test_join_computed_expr_right_key(streaming_engine_factory) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't remember exactly how we run these tests multi-rank. But we should ensure this test is run multi-rank.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the Ray variation will run two ranks on the same visible device. |
||
| """Join on a computed key expression.""" | ||
| engine = streaming_engine_factory( | ||
| StreamingOptions( | ||
| target_partition_size=1, | ||
| max_rows_per_partition=4, | ||
| broadcast_limit=1, # Disable broadcast joins | ||
| ), | ||
| ) | ||
| if engine.nranks < 2: | ||
| pytest.skip("bug only manifests on 2+ ranks") | ||
|
|
||
| zip_prefixes = ["10", "20", "30", "40"] | ||
| full_zips = ["10001", "20001", "30001", "40001"] | ||
| reps = 4 | ||
|
|
||
| # Start with joins on concrete column references | ||
| # to establish left and right partitioning metadata. | ||
| left_a = pl.LazyFrame( | ||
| { | ||
| "zip_prefix": zip_prefixes * reps, | ||
| "val_a": list(range(len(zip_prefixes) * reps)), | ||
| } | ||
| ) | ||
| left_b = pl.LazyFrame( | ||
| { | ||
| "zip_prefix": zip_prefixes * reps, | ||
| "val_b": list(range(100, 100 + len(zip_prefixes) * reps)), | ||
| } | ||
| ) | ||
| left = left_a.join(left_b, on="zip_prefix", how="inner") | ||
|
|
||
| right_a = pl.LazyFrame( | ||
| { | ||
| "full_zip": full_zips * reps, | ||
| "val_c": list(range(200, 200 + len(full_zips) * reps)), | ||
| } | ||
| ) | ||
| right_b = pl.LazyFrame( | ||
| { | ||
| "full_zip": full_zips * reps, | ||
| "val_d": list(range(300, 300 + len(full_zips) * reps)), | ||
| } | ||
| ) | ||
| right = right_a.join(right_b, on="full_zip", how="inner") | ||
|
|
||
| # Now join on a computed key expression. | ||
| # This should not silently drop rows across ranks | ||
| q = left.join( | ||
| right, | ||
| left_on="zip_prefix", | ||
| right_on=pl.col("full_zip").str.slice(0, 2), | ||
| ) | ||
| assert_gpu_result_equal(q, engine=engine, check_row_order=False) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what circumstances to we not want this?
names_to_indicesis used to convert name references into a column indices of a table. So by definition, I think, it can't be used to if the namedexpr isn't referring to a column?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on whether the
schemawe are indexing on corresponds the input or the output of the expressions innames.If the schema references the output DataFrame, then it's fine for the expressions to be non-concrete - The output of the expressions are concrete columns. If the schema references the input DataFrame, then the expression must be concrete.
When we check if the input DataFrame is already partitioned correctly, we must pass in this
concrete_prefix=Trueoption.