[GLUTEN-12375][VL][UT] Do not normalize string literals as ExprIds in plan stability suites#12521
Open
PerumalsamyR wants to merge 1 commit into
Open
[GLUTEN-12375][VL][UT] Do not normalize string literals as ExprIds in plan stability suites#12521PerumalsamyR wants to merge 1 commit into
PerumalsamyR wants to merge 1 commit into
Conversation
… plan stability suites glutenNormalizeIds() treats every '#<digits>' token in the explain text as an ExprId. String constants from the benchmark queries, such as Brand#12 in TPCH q19 or 'scholaramalgamalg apache#14' in TPCDS q53, match the same pattern, so they were renumbered as if they were ExprIds. Whether a literal token collides with a real ExprId depends on the JVM's ExprId counter, so the normalized output was stable only when a whole module runs in one JVM; an isolated suite run normalizes differently and fails against the committed goldens. Extract the string literals containing '#<digits>' from the query text and shield their occurrences in the explain output from ExprId normalization. Golden files are updated accordingly: protected literals now keep their true values and the remaining ExprIds are renumbered without the slots the literals used to occupy. Also move the pure normalization helpers to a companion object and add GlutenNormalizeIdsSuite covering the collision case. Fixes apache#12375
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes instability in Gluten’s Spark plan-stability test suites by preventing string literals that happen to contain #<digits> (e.g., TPCH Brand#12) from being mistakenly normalized as ExprIds, making golden comparisons consistent across JVM/session differences.
Changes:
- Updated
glutenNormalizeIdsto accepthashLiteralsextracted from the SQL text and mask their#fragments during ExprId normalization. - Refactored normalization helpers into the
GlutenPlanStabilityTestTraitcompanion object and addedGlutenNormalizeIdsSuiteunit tests (Spark 4.0/4.1). - Updated affected TPCH/TPCDS plan-stability golden explain outputs to reflect the corrected normalization behavior.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenPlanStabilitySuite.scala | Masks #<digits> occurrences for query string literals during plan normalization to stabilize goldens. |
| gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenNormalizeIdsSuite.scala | Adds unit tests covering normalization ordering, plan id handling, literal extraction, and stability. |
| gluten-ut/spark41/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-v1_4/q63/explain.txt | Updates golden explain output to preserve protected literal values and new ExprId renumbering. |
| gluten-ut/spark41/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-v1_4/q63.sf100/explain.txt | Same as above for sf100 variant. |
| gluten-ut/spark41/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-v1_4/q53.sf100/explain.txt | Same as above for q53 sf100. |
| gluten-ut/spark41/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-modified/q63.sf100/explain.txt | Updates modified-suite golden to match new literal shielding and renumbering. |
| gluten-ut/spark41/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-modified/q53.sf100/explain.txt | Updates modified-suite golden to match new literal shielding and renumbering. |
| gluten-ut/spark41/src/test/resources/backends-velox/gluten-tpch-plan-stability/q19/explain.txt | Restores true TPCH brand literal values (Brand#12/#23/#34) in golden output. |
| gluten-ut/spark41/src/test/resources/backends-velox/gluten-tpch-plan-stability/q17/explain.txt | Restores true TPCH brand literal value in golden output. |
| gluten-ut/spark41/src/test/resources/backends-velox/gluten-tpch-plan-stability/q16/explain.txt | Restores true TPCH brand literal value in golden output. |
| gluten-ut/spark40/src/test/scala/org/apache/spark/sql/GlutenPlanStabilitySuite.scala | Same normalization change as Spark 4.1 suite to keep Spark 4.0 goldens stable. |
| gluten-ut/spark40/src/test/scala/org/apache/spark/sql/GlutenNormalizeIdsSuite.scala | Same unit tests as Spark 4.1 module for the extracted normalization helpers. |
| gluten-ut/spark40/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-v1_4/q63/explain.txt | Updates Spark 4.0 golden explain output for the corrected normalization behavior. |
| gluten-ut/spark40/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-v1_4/q63.sf100/explain.txt | Same as above for sf100 variant. |
| gluten-ut/spark40/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-v1_4/q53.sf100/explain.txt | Same as above for q53 sf100. |
| gluten-ut/spark40/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-modified/q63.sf100/explain.txt | Updates modified-suite golden to match new literal shielding and renumbering. |
| gluten-ut/spark40/src/test/resources/backends-velox/tpcds-plan-stability/gluten-approved-plans-modified/q53.sf100/explain.txt | Updates modified-suite golden to match new literal shielding and renumbering. |
| gluten-ut/spark40/src/test/resources/backends-velox/gluten-tpch-plan-stability/q19/explain.txt | Restores true TPCH brand literal values (Brand#12/#23/#34) in Spark 4.0 golden output. |
| gluten-ut/spark40/src/test/resources/backends-velox/gluten-tpch-plan-stability/q17/explain.txt | Restores true TPCH brand literal value in Spark 4.0 golden output. |
| gluten-ut/spark40/src/test/resources/backends-velox/gluten-tpch-plan-stability/q16/explain.txt | Restores true TPCH brand literal value in Spark 4.0 golden output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
glutenNormalizeIds()in the Gluten plan stability suites treats every#<digits>token in the explain text as an ExprId. String constants from the benchmark queries match the same pattern —Brand#12/Brand#23/Brand#34in TPCH q19,Brand#45in q16,Brand#23in q17, and thei_brandvalues likescholaramalgamalg #14in TPCDS q53/q63 — so they were renumbered as if they were ExprIds.Whether such a literal token collides with a real ExprId depends on the JVM's ExprId counter at the time the plan is printed: with the whole module running in one JVM the counter is large and the literals get their own normalization slots, but in an isolated suite run the counter is small (e.g.
p_brand#12vs the literalBrand#12share the#12token), the equivalence classes merge, and the output no longer matches the committed goldens. This is the long-term fix requested in #12375.Changes:
glutenNormalizeIdsnow takes the string literals containing#<digits>(extracted from the query text withgetHashLiterals) and shields their occurrences in the explain output from ExprId normalization by masking the#during the rewrite. Normalization output no longer depends on the JVM's ExprId counter.GlutenNormalizeIdsSuite(new) covers encounter-order normalization, plan-id/_pre_handling, literal extraction, CHAR-padded literals, and the collision/stability case.The golden update was derived mechanically from the committed goldens: the old normalization is a bijective, encounter-order-preserving renumbering, so applying the new normalizer to the golden with the literal values restored reproduces exactly what the new normalizer emits for the raw explain text. The transform asserted that literal slots were never merged with attribute ExprIds in the committed goldens (they were not, for all 22 files) and that the result is a fixed point of the new normalizer. The plan-stability suites in CI verify the result end to end.
Fixes #12375
How was this patch tested?
GlutenNormalizeIdsSuitepasses locally (dev/run-scala-test.sh -Pjava-17,spark-4.0,scala-2.13,backends-velox,hadoop-3.3,spark-ut -pl gluten-ut/spark40 -s org.apache.spark.sql.GlutenNormalizeIdsSuite), including a test that reproduces the isolated-run vs shared-JVM divergence without the fix and asserts stability with it.GlutenTPCHPlanStabilitySuite,GlutenTPCDS*PlanStabilitySuite) validate the updated goldens in CI.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code