Fix SPJ runtime partition validation [databricks][reduced-it] - #15924
Fix SPJ runtime partition validation [databricks][reduced-it]#15924amahussein wants to merge 4 commits into
Conversation
Fixes NVIDIA#15839 Validate runtime-filtered scan partition keys against the scan's original full partitioning instead of the SPJ-facing projected view. Add a release-gated Iceberg regression that proves the partition-filter and dynamic-pruning plan shape. Signed-off-by: Ahmed Hussein <ahussein@nvidia.com>
|
build |
|
|
build |
| @pytest.mark.skipif( | ||
| not ( | ||
| (is_spark_40x() and _is_spark_patch_at_least(spark_version(), 5)) | ||
| or (is_spark_41x() and _is_spark_patch_at_least(spark_version(), 4)) | ||
| ), | ||
| reason="SPARK-58783 was fixed in Spark 4.0.5 and 4.1.4; Spark 4.2+ is unaffected") |
There was a problem hiding this comment.
As written, CI compiles the change but never exercises the production code change
Could we add a current-release test that separately expects the CPU exception and verifies the successful GPU result and plan?
There was a problem hiding this comment.
Added an affected-release regression to the existing Iceberg extra-classpath premerge job, the only premerge path that executes Iceberg tests. It asserts the CPU exception, then verifies the GPU result, DPP values, and exchange-free SPJ plan. On Spark 4.0.2, reverting this fix made it fail and restoring it passed; premerge will run Spark 4.0.1. Fixed releases use parity.
| "spark.sql.optimizer.dynamicPartitionPruning.fallbackFilterRatio": "10", | ||
| "spark.sql.sources.v2.bucketing.enabled": "true", | ||
| "spark.sql.sources.v2.bucketing.pushPartValues.enabled": "true", | ||
| "spark.sql.sources.v2.bucketing.partition.filter.enabled": "true", |
There was a problem hiding this comment.
Could we also cover the projected trailing-join-key path? This case uses one INT partition key and exercises only the common-value intersection. SPARK-58783 also fixes joinKeyPositions projections; with partition keys such as
(INT, STRING) and a join on the trailing string, the old code can interpret the first raw field using the projected StringType and fail with a ClassCastException. A DPP test using a non-leading join key and asserting
joinKeyPositions would cover that distinct part of the fix.
There was a problem hiding this comment.
Added an (INT, STRING) identity-partitioned DPP case to that premerge job. It joins on the trailing string key and asserts joinKeyPositions == [1], pruning and common partition values, GPU rows, and exchange-free SPJ. Reverting this fix makes its GPU leg fail. The measured CPU path fails in Iceberg's partition-data check before Spark's equivalent cast; fixed releases use parity.
Review feedback on NVIDIA#15924. The parity test added with the receiver fix is gated to Spark 4.0.5+ or 4.1.4+, neither of which exists, so CI compiled the production change without ever executing it. Separately, the joinKeyPositions projection route that SPARK-58783 also repairs had no plugin coverage of any kind. test_iceberg_spj_partition_filter_with_runtime_filter_cpu_fails_gpu_succeeds runs on affected Spark 4.0.x and 4.1.x releases. It asserts the CPU runtime-filtering exception, then requires the GPU to return the expected row with evaluated DPP values and an exchange-free SPJ plan. It retires itself once a fixed release ships, at which point the existing parity test takes over, so the asymmetric assertion never needs to be undone by hand. test_iceberg_spj_runtime_filter_with_trailing_join_key partitions the left table by (dept_id, data) and joins on the trailing string key, so joinKeyPositions is [1] and the pre-fix receiver wraps a raw INT key with the projected StringType. Asserting commonPartitionValues keeps the case on the two-sided route and away from open issue NVIDIA#15338. On affected releases the CPU leg fails inside Iceberg's PartitionData type check, which fires before Spark's own cast, so the matcher accepts either message. Both regressions are added to the Iceberg extra-classpath premerge job, which is the only premerge path that executes Iceberg tests. Verified on Spark 4.0.2 with Iceberg 1.10.1 in both the normal and extra-classpath layouts. Reverting the receiver fix makes both GPU legs fail and restoring it makes both pass, so these exercise the production change rather than only its plan shape. Premerge itself runs Spark 4.0.1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
|
build |
amahussein
left a comment
There was a problem hiding this comment.
Thanks @gerashegalov
Addressed review comments
| "spark.sql.optimizer.dynamicPartitionPruning.fallbackFilterRatio": "10", | ||
| "spark.sql.sources.v2.bucketing.enabled": "true", | ||
| "spark.sql.sources.v2.bucketing.pushPartValues.enabled": "true", | ||
| "spark.sql.sources.v2.bucketing.partition.filter.enabled": "true", |
There was a problem hiding this comment.
Added an (INT, STRING) identity-partitioned DPP case to that premerge job. It joins on the trailing string key and asserts joinKeyPositions == [1], pruning and common partition values, GPU rows, and exchange-free SPJ. Reverting this fix makes its GPU leg fail. The measured CPU path fails in Iceberg's partition-data check before Spark's equivalent cast; fixed releases use parity.
| @pytest.mark.skipif( | ||
| not ( | ||
| (is_spark_40x() and _is_spark_patch_at_least(spark_version(), 5)) | ||
| or (is_spark_41x() and _is_spark_patch_at_least(spark_version(), 4)) | ||
| ), | ||
| reason="SPARK-58783 was fixed in Spark 4.0.5 and 4.1.4; Spark 4.2+ is unaffected") |
There was a problem hiding this comment.
Added an affected-release regression to the existing Iceberg extra-classpath premerge job, the only premerge path that executes Iceberg tests. It asserts the CPU exception, then verifies the GPU result, DPP values, and exchange-free SPJ plan. On Spark 4.0.2, reverting this fix made it fail and restoring it passed; premerge will run Spark 4.0.1. Fixed releases use parity.
| return _partition_filter_runtime_filter_query( | ||
| spark, left_table, right_table, dim_table) | ||
|
|
||
| def assert_plan(plan): |
There was a problem hiding this comment.
After the merge from main, assert_cpu_and_gpu_are_equal_collect_with_capture calls this callback with (cpu_plan, gpu_plan). Once 4.0.5 or 4.1.4 enables this test, the one-argument function will fail with TypeError before checking the plan. Could this be def assert_plan(_cpu_plan, plan)?
| join_with_runtime_filter, | ||
| conf=conf, | ||
| require_non_empty=True, | ||
| gpu_plan_assertion=assert_plan) |
There was a problem hiding this comment.
After the merge from main, assert_cpu_and_gpu_are_equal_collect_with_capture calls this callback with (cpu_plan, gpu_plan). Once 4.0.5 or 4.1.4 enables this test, the one-argument function will fail with TypeError before checking the plan. Could this be def assert_plan(_cpu_plan, plan)?
Fixes #15839.
Description
During runtime filtering,
GpuBatchScanExec.filteredPartitionsvalidates full scan keys against SPJ-facingoutputPartitioning, whose values may be intersected or projected. Valid scan-owned keys can consequently be rejected as new.This ports Spark's SPARK-58783 receiver correction to shared
spark340andspark350db143: validation now usessuper.outputPartitioning, preserving scan-native keys while later SPJ projection and grouping remain unchanged. Spark 3.4 and 3.5 cannot reach the false rejection; aligningspark340does not change conforming prune-only behavior.On affected Spark 4.0.x and 4.1.x releases,
test_iceberg_spj_partition_filter_with_runtime_filter_cpu_fails_gpu_succeedsasserts the upstream CPU exception, then verifies GPU results, evaluated DPP values, and an exchange-free GPU SPJ plan. The fixed-release parity test remains gated to Spark 4.0.5+ or 4.1.4+.test_iceberg_spj_runtime_filter_with_trailing_join_keyadds an(INT, STRING)identity-partitioned DPP case joining on the trailing string key. It asserts projectedjoinKeyPositions, pruning and common partition values, expected GPU rows, and exchange-free SPJ execution. Affected releases assert the observed CPU-side Iceberg partition-data type-check failure; fixed releases require CPU/GPU parity. Both regressions are selected in the only premerge Iceberg job, its extra-classpath smoke list.Affected Apache Spark profiles served by the shared source intentionally differ from their buggy CPU behavior. Mapped Databricks builds receive the same correction, but their CPU/GPU parity remains unverified.
Validation passed a Scala 2.13 buildver 402 install, RAT, Scalastyle, resource-nesting lint, and static checks. Spark 4.0.2 with Iceberg 1.10.1 passed both regressions in normal and CI extra-classpath layouts. Reverting the receiver fix made both GPU legs fail; restoring it made both pass. Premerge itself runs Spark 4.0.1.
No public API or configuration changes. Performance is unaffected: validation only selects different existing metadata.
AI assistance was used to implement and review this change.
Checklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance