[FEA] Enable per-expression legacy AST projection [databricks] [fast-ut] - #15377
[FEA] Enable per-expression legacy AST projection [databricks] [fast-ut]#15377thirtiseven wants to merge 10 commits into
Conversation
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
|
@greptile full review |
Greptile SummaryThis PR replaces the all-or-nothing
Confidence Score: 5/5Safe to merge. Resource management (compiled expressions, intermediate Tables) is correct, OOM split-retry is preserved via GpuTieredProject, and per-expression routing logic is well-tested at both unit and integration level. The core change — replacing the all-or-nothing GpuProjectAstExec with per-expression GpuProjectAstExpression markers — is architecturally clean and the implementation handles the key correctness concerns: lazy compilation with synchronized access, AutoCloseable + task-completion cleanup, shared Table creation guarded by withResource, and correct CSE backward-propagation through rewrapAstTiers. The two observations are minor explain verbosity and a test coverage gap in the Python integration tests; neither affects runtime correctness. Files Needing Attention: No files require special attention. The integration test null-literal routing case could use a mixed-type companion test for stronger isolation, but this is not a blocker. Important Files Changed
Reviews (8): Last reviewed commit: "Merge branch 'main' into legacy-ast-per-..." | Re-trigger Greptile |
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
|
NOTE: release/26.08 has been created from main. Please retarget your PR to release/26.08 if it should be included in the release. |
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
igorpeshansky
left a comment
There was a problem hiding this comment.
LGTM
modulo one remaining missing comment.
|
build |
|
build |
|
build |
Related to #8954.
Description
The motivation for this PR is to prepare for AST JIT work.
Legacy AST Project execution is currently all-or-nothing: if any top-level Project output cannot be represented by the legacy AST backend, eligible sibling outputs cannot use AST either.
This change selects the legacy AST backend independently for each top-level Project expression. Supported fixed-width outputs use AST while unsupported outputs in the same Project continue through the regular GPU expression path. Top-level null literals also remain on the regular projection path so their outputs can reuse its cached null vector. It uses the existing
spark.rapids.sql.projectAstEnabledconfiguration and does not change result semantics or add a user-facing configuration.This is independent of the experimental AST JIT work in #15312. It refactors the existing legacy AST path and does not depend on AST JIT, LTO, or precompiled fragments.
This also changes physical-plan rendering: AST projections now appear under GpuProject as
AST(...) AS xinstead of using a dedicatedGpuProjectAstExecnode. This does not change result semantics, APIs, or configuration.Implementation details:
GpuProjectAstExpression.Tableacross all AST outputs evaluated for the same tier and batch.Performance testing:
local[4], plugin26.08.0-SNAPSHOT, and 2 NVIDIA RTX 5880 Ada Generation GPUs.LIBCUDF_JIT_ENABLED=0isolates the legacy AST backend. AST off/on order and case order are reversed on alternating iterations.opTimeLegacy, accumulated across tasks, so it is not directly comparable to E2E wall-clock time.broad_unique_ast[0]/[84]whole_output_duplicates[0,0]/[42,0]cheap_partial_cse[0,0]/[1,84]expensive_partial_cse[0,0]/[1,84]mixed_half[0]/[42]No median regression was observed in the five main benchmark cases in either E2E or Project op time.
Top-level literal routing follow-up
A targeted follow-up compared top-level literals on the regular and AST paths. It used 20 million rows, 84 Project outputs, two warmups, and 15 alternating measured iterations. The benchmark directly executed and consumed the columnar
GpuProject, avoiding Catalyst constant folding and unrelated aggregate, shuffle, or ColumnarToRow work. Speedup is regular / AST; values above 1 mean AST is faster.literal_onlymixed_ast_literalsnull_literal_onlyNon-null literals were neutral in E2E time in both the literal-only and mixed workloads, so there is no evidence for excluding all top-level literals from AST. Null literals were different: the regular projection was 4.1x faster E2E and 49.6x faster in Project op time because it can reuse its cached null vector across outputs, whereas per-expression AST evaluates the null outputs independently. Based on these results, this PR keeps only top-level null literals on the regular projection path and leaves non-null literals AST-eligible.
TPC-H/TPC-DS coverage
We also performed physical-plan sweeps over stock TPC-H/NDS-H and TPC-DS workloads.
These workloads therefore spend nearly all of their time in unrelated scans, joins, and aggregations and do not provide a meaningful performance signal for this change. The performance results above use purpose-built AST-only and mixed-expression Projects so that the affected execution path is exercised directly, while still materializing every projected expression.
Checklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance