Skip to content

Commit 2e263a6

Browse files
committed
Merge branch 'main' into regex-transpiler-bugfixes
Signed-off-by: Igor Peshansky <ipeshansky@nvidia.com>
2 parents 54702b9 + 09ed1f3 commit 2e263a6

12 files changed

Lines changed: 192 additions & 85 deletions

File tree

integration_tests/README.md

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -563,13 +563,17 @@ partition type to keep memory usage manageable.
563563

564564
#### Iceberg REST catalog write compression
565565

566-
Some REST catalog deployments can apply a catalog-side default Parquet compression codec that
567-
is not supported by the RAPIDS GPU writer. The REST catalog CI sets table defaults for data
568-
and delete files to use `zstd`, which is supported by the GPU writer:
566+
Older Iceberg REST clients, including 1.6.x, do not apply client-side
567+
`spark.sql.catalog.*.table-default.*` settings when creating REST tables. The resulting tables
568+
can use a default Parquet compression codec that is not supported by the RAPIDS GPU writer.
569+
REST catalog tests therefore add explicit table properties for data and delete files to use
570+
`zstd`, which is supported by the GPU writer:
569571

570-
```shell
571-
"PYSP_TEST_spark_sql_catalog_spark__catalog_table-default_write_parquet_compression-codec=zstd"
572-
"PYSP_TEST_spark_sql_catalog_spark__catalog_table-default_write_delete_parquet_compression-codec=zstd"
572+
```sql
573+
TBLPROPERTIES (
574+
'write.parquet.compression-codec' = 'zstd',
575+
'write.delete.parquet.compression-codec' = 'zstd'
576+
)
573577
```
574578

575579
### Run Apache iceberg s3tables tests

integration_tests/src/main/python/delta_lake_test.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -693,8 +693,11 @@ def setup_table(spark):
693693
f"b={b_size}, max_split={max_split}")
694694

695695
a_tail_start = a_size - a_tail
696-
a_midpoints = parquet_row_group_midpoints(a_path)
697-
b_midpoints = parquet_row_group_midpoints(b_path)
696+
a_midpoints, b_midpoints = with_cpu_session(
697+
lambda spark: (
698+
parquet_row_group_midpoints(spark, a_path),
699+
parquet_row_group_midpoints(spark, b_path),
700+
))
698701
assert any(a_tail_start <= midpoint < a_size for midpoint in a_midpoints), (
699702
f"A tail split [{a_tail_start}, {a_size}) has no row-group midpoint; "
700703
f"midpoints={a_midpoints}")

integration_tests/src/main/python/iceberg/__init__.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323

2424
import pytest
2525

26-
from conftest import spark_jvm
26+
from conftest import is_iceberg_rest_catalog, spark_jvm
2727
from data_gen import *
2828
from spark_session import is_iceberg_supported_spark, with_cpu_session
2929

@@ -297,10 +297,19 @@ def schema_to_ddl(spark, schema):
297297

298298

299299
# Base table properties applied to every Iceberg test table.
300-
# Disables the fanout writer to prevent OOM in CI. S3TablesCatalog does not
300+
# Disables the fanout writer to prevent OOM in CI. S3TablesCatalog does not
301301
# honor catalog-level table-default properties, so this must be set per table.
302302
_BASE_TBLPROPS = {'write.spark.fanout.enabled': False}
303303

304+
# Older Iceberg REST clients, including 1.6.x, do not apply catalog table-default
305+
# properties when creating a table. Apply supported codecs directly to REST test
306+
# tables so writes do not fall back to gzip and then to CPU.
307+
if is_iceberg_rest_catalog():
308+
_BASE_TBLPROPS.update({
309+
'write.parquet.compression-codec': 'zstd',
310+
'write.delete.parquet.compression-codec': 'zstd',
311+
})
312+
304313

305314
def _to_tblprops_str(props: dict) -> dict:
306315
"""Convert property values to their SQL-safe string representation."""

integration_tests/src/main/python/parquet_test.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1020,8 +1020,11 @@ def parquet_file_info_by_part(spark):
10201020
f"b={b_size}, max_split={max_split}")
10211021

10221022
a_tail_start = a_size - a_tail
1023-
a_midpoints = parquet_row_group_midpoints(a_path)
1024-
b_midpoints = parquet_row_group_midpoints(b_path)
1023+
a_midpoints, b_midpoints = with_cpu_session(
1024+
lambda spark: (
1025+
parquet_row_group_midpoints(spark, a_path),
1026+
parquet_row_group_midpoints(spark, b_path),
1027+
))
10251028
assert any(a_tail_start <= midpoint < a_size for midpoint in a_midpoints), (
10261029
f"A tail split [{a_tail_start}, {a_size}) has no row-group midpoint; "
10271030
f"midpoints={a_midpoints}")

integration_tests/src/main/python/parquet_test_utils.py

Lines changed: 14 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -12,29 +12,18 @@
1212
# See the License for the specific language governing permissions and
1313
# limitations under the License.
1414

15-
from urllib.parse import urlparse
16-
17-
import pyarrow.fs as pa_fs
18-
import pyarrow.parquet as pa_pq
19-
20-
21-
def parquet_row_group_midpoints(path):
15+
def parquet_row_group_midpoints(spark, path):
2216
"""Returns an approximate byte midpoint for each Parquet row group."""
23-
if urlparse(path).scheme:
24-
filesystem, path = pa_fs.FileSystem.from_uri(path)
25-
meta = pa_pq.read_metadata(path, filesystem=filesystem)
26-
else:
27-
meta = pa_pq.read_metadata(path)
28-
midpoints = []
29-
for rg_index in range(meta.num_row_groups):
30-
row_group = meta.row_group(rg_index)
31-
first_col = row_group.column(0)
32-
start = first_col.data_page_offset
33-
dict_offset = first_col.dictionary_page_offset
34-
if dict_offset is not None and dict_offset > 0:
35-
start = min(start, dict_offset)
36-
total_size = 0
37-
for col_index in range(row_group.num_columns):
38-
total_size += row_group.column(col_index).total_compressed_size
39-
midpoints.append(start + total_size // 2)
40-
return midpoints
17+
jvm = spark.sparkContext._jvm
18+
hadoop_conf = spark.sparkContext._jsc.hadoopConfiguration()
19+
hadoop_path = jvm.org.apache.hadoop.fs.Path(path)
20+
reader = jvm.org.apache.parquet.hadoop.ParquetFileReader.open(
21+
hadoop_conf, hadoop_path)
22+
try:
23+
blocks = reader.getFooter().getBlocks()
24+
return [
25+
block.getStartingPos() + block.getCompressedSize() // 2
26+
for block in blocks
27+
]
28+
finally:
29+
reader.close()

integration_tests/src/main/python/private_optimizer_README.md

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ do **two** things:
1616
the rule.
1717
2. Assert a **per-rule plan marker** that appears only when the rule fires, so
1818
the test FAILS if the conf flip is a no-op (wrong conf, wrong query shape, or
19-
the private jar is not loaded).
19+
the private jar is not loaded). A runtime-specific no-op path instead uses
20+
`assert_rule_skipped` with a required marker proving that the guarded plan
21+
shape was reached.
2022

2123
## Layout
2224

@@ -51,7 +53,12 @@ All modules carry `@pytest.mark.private_optimizer`, so the whole area runs with:
5153
such as broadcast thresholds or AQE toggles).
5254
- `assert_rule_fires(fn, on_conf, off_conf, marker, physical=False)`
5355
— runs the OFF-CPU vs ON-GPU comparison and the plan-marker check.
54-
3. **Decorate** the test with `@pytest.mark.private_optimizer`. Add `@approximate_float` only when the rule
56+
- `assert_rule_skipped(fn, on_conf, off_conf, marker, physical=False,
57+
required_on_markers=())`
58+
— runs the same comparison but asserts a runtime-specific no-op path where
59+
the marker is absent from both plans.
60+
3. **Decorate** the test with `@pytest.mark.private_optimizer`. Add
61+
`@approximate_float` only when the rule
5562
changes floating-point evaluation order — that is enough for floating-point
5663
tolerance, since `assert_rule_fires` routes result comparison through
5764
`assert_equal_with_local_sort`, whose `get_float_check()` honors it.
@@ -95,6 +102,12 @@ executed plan); otherwise the optimized plan is checked. Examples in this area:
95102
`named_struct(c_0,` (merged subquery scan), `coalesced and skewed` (skew
96103
reader).
97104

105+
When a known runtime or plan shape intentionally skips a rule, add a separate
106+
test with `assert_rule_skipped` instead of weakening the positive marker test.
107+
That skipped-path test must explain the runtime/shape reason, still compare
108+
OFF-CPU vs ON-GPU results, and use `required_on_markers` to prove the expected
109+
plan shape was reached.
110+
98111
### When a row-count / aggregate-only check is acceptable
99112

100113
The default is a full row-by-row `OFF-CPU == ON-GPU` comparison. A reduced check
@@ -109,8 +122,13 @@ rule actually ran. Do **not** weaken the marker check to make a test pass.
109122
### Compatibility across Spark versions and runtimes
110123

111124
The private plugin is built for **Spark 3.3.0 and later** (private core build
112-
matrix: 330..411 plus the `400db173` Databricks buildver). Within that matrix all four current rules apply on every
113-
runtime, including Databricks, so we do **not** add per-runtime skips.
125+
matrix: 330..411 plus Databricks buildvers). The rules are loaded throughout
126+
that matrix, but a rule can intentionally guard a specific runtime plan shape.
127+
For example, `OptimizeSkewedBHJJoinRule` skips Databricks executor-broadcast
128+
joins because its streamed-side skew rewrite does not apply to that shape.
129+
Keep positive rule-fire coverage on supported shapes and add a separate
130+
`assert_rule_skipped` test for an intentional guard; do not skip the runtime's
131+
coverage entirely.
114132

115133
If a future rule is genuinely unsupported on some runtime/version, express it
116134
where the source actually gates it:

integration_tests/src/main/python/private_optimizer_common.py

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
The covered rules are semantics-preserving but default-off or otherwise dormant
2222
in public IT.
2323
Comparing a same-conf CPU run against a same-conf GPU run cannot tell whether a
24-
rule actually fired, so every test built on these helpers does two things:
24+
rule actually fired, so positive-path tests built on these helpers do two things:
2525
2626
1. Compares an OFF-rule CPU baseline against an ON-rule GPU run. Because the
2727
rules preserve semantics, OFF-CPU == ON-GPU proves both data correctness
@@ -30,19 +30,21 @@
3030
the test FAILS if the conf flip is a no-op (wrong conf, wrong query shape,
3131
or private jar not loaded).
3232
33+
Runtime-specific no-op paths use ``assert_rule_skipped`` instead. Those tests
34+
require a marker for the guarded plan shape and assert that the rule marker is
35+
absent while preserving the same OFF-CPU vs ON-GPU result comparison.
36+
3337
See ``private_optimizer_README.md`` for how to add a new rule module.
3438
"""
3539

3640
from asserts import assert_equal_with_local_sort
3741
from spark_session import with_cpu_session, with_gpu_session
3842

3943
# The private optimizer rules ship in the spark-rapids-private plugin, which is
40-
# built only for Spark 3.3.0 and later (see the private core pom build matrix:
41-
# 330..411 plus the Databricks 400db173 buildver). Runtimes within the matrix
42-
# (including Databricks) are all supported for these four rules, so we do not
43-
# add per-runtime skips here. A rule that becomes unsupported on some future
44-
# runtime is caught by the plan-marker assertion below (it fails loudly instead
45-
# of passing silently), not by guessing a version here.
44+
# built only for Spark 3.3.0 and later. Rules are loaded across the supported
45+
# Apache and Databricks build matrix. An intentional runtime-plan guard belongs
46+
# in the rule itself and is covered here with assert_rule_skipped; do not hide
47+
# that path with a blanket runtime skip.
4648

4749
PRIVATE_OPTIMIZER_BASE_CONF = {
4850
"spark.rapids.sql.private.enabled": "true",
@@ -91,3 +93,28 @@ def assert_rule_fires(fn, on_conf, off_conf, marker, physical=False):
9193
"marker '%s' present with rule OFF, not a valid discriminator\n%s" % (marker, off_plan)
9294

9395
assert_equal_with_local_sort(cpu_rows, gpu_rows)
96+
97+
98+
def assert_rule_skipped(fn, on_conf, off_conf, marker, physical=False, required_on_markers=()):
99+
"""OFF-rule CPU baseline vs ON-rule GPU run for a runtime-specific no-op path.
100+
101+
marker must be absent from both plans; results of the OFF-CPU and ON-GPU
102+
runs must match. Use this only when the rule is expected to skip itself for
103+
a known runtime or plan shape. Every required_on_markers entry must still be
104+
present in the ON plan to prove the test reached the expected shape.
105+
"""
106+
cpu_rows, off_plan = with_cpu_session(
107+
lambda s: collect_and_plan(s, fn, physical), conf=off_conf)
108+
gpu_rows, on_plan = with_gpu_session(
109+
lambda s: collect_and_plan(s, fn, physical), conf=on_conf)
110+
111+
assert marker not in on_plan, \
112+
"runtime skip failed: marker '%s' present with rule ON\n%s" % (marker, on_plan)
113+
assert marker not in off_plan, \
114+
"marker '%s' present in OFF-CPU baseline, not a valid skipped-path check\n%s" % (
115+
marker, off_plan)
116+
for required_marker in required_on_markers:
117+
assert required_marker in on_plan, \
118+
"required marker '%s' missing from rule ON plan\n%s" % (required_marker, on_plan)
119+
120+
assert_equal_with_local_sort(cpu_rows, gpu_rows)

integration_tests/src/main/python/private_optimizer_skewed_bhj_join_test.py

Lines changed: 77 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -16,32 +16,24 @@
1616

1717
from private_optimizer_common import (
1818
assert_rule_fires,
19+
assert_rule_skipped,
1920
private_optimizer_conf,
2021
)
2122
from spark_session import is_databricks_runtime
2223

2324

24-
@pytest.mark.private_optimizer
25-
@pytest.mark.skipif(
26-
is_databricks_runtime(),
27-
reason="Databricks executor-broadcast AQE can put the materialized shuffle on the "
28-
"BHJ build side; this marker test covers streamed-side skew split. "
29-
"See https://github.com/NVIDIA/cudf-spark/issues/15136")
30-
def test_optimize_skewed_bhj_join(spark_tmp_path):
31-
"""OptimizeSkewedBHJJoinRule splits a skewed partition on the streamed side
32-
of an AQE broadcast hash join. Needs a runtime broadcast (static
33-
autoBroadcastJoinThreshold=-1, adaptive.autoBroadcastJoinThreshold=10m) so
34-
the streamed side is a materialized shuffle stage, plus small skew
35-
thresholds. Marker: the shuffle reader is 'coalesced and skewed'.
25+
SKEWED_BHJ_MARKER = "coalesced and skewed"
26+
# The DB GpuBroadcastHashJoinExec plan ends with isNullAwareAntiJoin and
27+
# executorBroadcast. This marker proves the shim guard's exact precondition was
28+
# reached rather than accepting any broadcast hash join as a skipped-rule path.
29+
DB_EXECUTOR_BROADCAST_MARKER = "GpuBuildRight, false, true"
30+
# The keyed exchange identifies the explicitly repartitioned streamed input.
31+
# GpuShuffleCoalesce alone is not specific because aggregate stages can add it.
32+
DB_STREAMED_SHUFFLE_MARKER = "GpuColumnarExchange gpuhashpartitioning(key1"
3633

37-
The rule additionally short-circuits in OptimizeSkewedBHJJoinRule.apply when
38-
AQEUtils.isOptimizeSkewBHJSupported is false, so if a future Spark/runtime
39-
drops support the rule becomes a no-op and the marker assertion below fails
40-
loudly rather than passing silently.
4134

42-
Validated with a small GLOBAL aggregate over the materialized skewed join;
43-
a GROUP BY on the skew key is intentionally avoided here."""
44-
conf_extra = {
35+
def _skewed_bhj_conf_extra():
36+
return {
4537
"spark.sql.adaptive.enabled": "true",
4638
"spark.sql.adaptive.skewJoin.enabled": "true",
4739
"spark.sql.autoBroadcastJoinThreshold": "-1",
@@ -53,19 +45,72 @@ def test_optimize_skewed_bhj_join(spark_tmp_path):
5345
"spark.sql.adaptive.localShuffleReader.enabled": "false",
5446
}
5547

56-
def fn(spark):
57-
spark.range(0, 2000, 1, 10).selectExpr(
58-
"CASE WHEN id < 1000 THEN 249 ELSE id END AS key2", "id AS value2"
59-
).createOrReplaceTempView("skewData2")
60-
spark.range(0, 1000, 1, 10).selectExpr(
61-
"CASE WHEN id < 250 THEN 249 WHEN id >= 750 THEN 1000 ELSE id END AS key1", "id AS value1"
62-
).createOrReplaceTempView("skewData1")
63-
return spark.sql(
64-
"SELECT count(*) AS cnt, min(value2) AS mn, max(value2) AS mx, sum(value1) AS sm "
65-
"FROM skewData1 JOIN skewData2 ON key1 = key2")
6648

49+
def _skewed_bhj_confs():
50+
conf_extra = _skewed_bhj_conf_extra()
6751
on = private_optimizer_conf(
68-
{"spark.rapids.sql.adaptive.skewJoin.broadcast.enabled": "true"}, extra_conf=conf_extra)
52+
{"spark.rapids.sql.adaptive.skewJoin.broadcast.enabled": "true"},
53+
extra_conf=conf_extra)
6954
off = private_optimizer_conf(
70-
{"spark.rapids.sql.adaptive.skewJoin.broadcast.enabled": "false"}, extra_conf=conf_extra)
71-
assert_rule_fires(fn, on, off, marker="coalesced and skewed", physical=True)
55+
{"spark.rapids.sql.adaptive.skewJoin.broadcast.enabled": "false"},
56+
extra_conf=conf_extra)
57+
return on, off
58+
59+
60+
def _skewed_bhj_global_agg(spark):
61+
spark.range(0, 2000, 1, 10).selectExpr(
62+
"CASE WHEN id < 1000 THEN 249 ELSE id END AS key2", "id AS value2"
63+
).createOrReplaceTempView("skewData2")
64+
spark.range(0, 1000, 1, 10).selectExpr(
65+
"CASE WHEN id < 250 THEN 249 WHEN id >= 750 THEN 1000 ELSE id END AS key1",
66+
"id AS value1"
67+
).repartition(100, "key1").createOrReplaceTempView("skewData1")
68+
return spark.sql(
69+
"SELECT /*+ BROADCAST(skewData2) */ "
70+
"count(*) AS cnt, min(value2) AS mn, max(value2) AS mx, sum(value1) AS sm "
71+
"FROM skewData1 JOIN skewData2 ON key1 = key2")
72+
73+
74+
@pytest.mark.private_optimizer
75+
@pytest.mark.skipif(
76+
is_databricks_runtime(),
77+
reason="The positive rule-fire assertion is Apache-only; the Databricks "
78+
"executor-broadcast guarded path is covered by the skipped-path test below. "
79+
"See https://github.com/NVIDIA/cudf-spark/issues/15136")
80+
def test_optimize_skewed_bhj_join(spark_tmp_path):
81+
"""OptimizeSkewedBHJJoinRule splits a skewed partition on the streamed side
82+
of an AQE broadcast hash join. The broadcast hint fixes the build side while
83+
the explicit key repartition makes the streamed side a materialized shuffle
84+
stage; small skew thresholds make the key 249 partition eligible to split.
85+
Marker: the shuffle reader is 'coalesced and skewed'.
86+
87+
The rule additionally short-circuits in OptimizeSkewedBHJJoinRule.apply when
88+
AQEUtils.isOptimizeSkewBHJSupported is false, so if a future Spark/runtime
89+
drops support the rule becomes a no-op and the marker assertion below fails
90+
loudly rather than passing silently.
91+
92+
Validated with a small GLOBAL aggregate over the materialized skewed join;
93+
a GROUP BY on the skew key is intentionally avoided here."""
94+
on, off = _skewed_bhj_confs()
95+
assert_rule_fires(_skewed_bhj_global_agg, on, off, marker=SKEWED_BHJ_MARKER,
96+
physical=True)
97+
98+
99+
@pytest.mark.private_optimizer
100+
@pytest.mark.skipif(
101+
not is_databricks_runtime(),
102+
reason="Databricks-only coverage for executor-broadcast AQE fallback. "
103+
"Apache runtime coverage is in test_optimize_skewed_bhj_join.")
104+
def test_optimize_skewed_bhj_join_skips_on_databricks_executor_broadcast(spark_tmp_path):
105+
"""Databricks executor-broadcast AQE is expected to skip the streamed-side
106+
rewrite even when the streamed input is a materialized skewed shuffle stage.
107+
Without the executor-broadcast shim guard this shape is eligible for the
108+
rewrite, as verified by the positive Apache test above. The streamed-side
109+
skew marker must remain absent while CPU and GPU results still match. The
110+
required markers verify a non-null-aware, executor-broadcast build-right join
111+
whose streamed input retains the expected GPU hash-partitioning exchange."""
112+
on, off = _skewed_bhj_confs()
113+
assert_rule_skipped(_skewed_bhj_global_agg, on, off, marker=SKEWED_BHJ_MARKER,
114+
physical=True, required_on_markers=(
115+
DB_EXECUTOR_BROADCAST_MARKER,
116+
DB_STREAMED_SHUFFLE_MARKER))

integration_tests/src/main/python/regexp_test.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,6 +309,7 @@ def test_re_replace_repetition():
309309
'REGEXP_REPLACE(a, "A{0,}", "PROD")',
310310
'REGEXP_REPLACE(a, "T?E?", "PROD")',
311311
'REGEXP_REPLACE(a, "A*", "PROD")',
312+
'REGEXP_REPLACE(a, "A+", "PROD")',
312313
'REGEXP_REPLACE(a, "A{0,5}", "PROD")',
313314
'REGEXP_REPLACE(a, "(A*)", "PROD")',
314315
'REGEXP_REPLACE(a, "(((A*)))", "PROD")',

0 commit comments

Comments
 (0)