[SPARK-52841][SQL][TESTS] Fix PlanStabilitySuite id normalization #51534
Conversation
|
|
||
| // Normalize the plan id in Exchange nodes. See `Exchange.stringArgs`. | ||
| // Normalize the plan ids in Exchange and Subquery nodes. |
There was a problem hiding this comment.
Thank you for supporting Subquery correctly.
dongjoon-hyun
changed the title
|
I added |
| @@ -78,8 +78,8 @@ trait PlanStabilitySuite extends DisableAdaptiveExecutionSuite { | |||
| } | |||
|
|
|||
| private val referenceRegex = "#\\d+".r | |||
| private val normalizeRegex = "#\\d+L?".r | |||
| private val planIdRegex = "plan_id=\\d+".r | |||
| private val exprIdRegexp = "(?<prefix>(?<!id=)#)\\d+L?".r | |||
There was a problem hiding this comment.
I'm not an expert on regex, can you give a bit more explanation about the fix here? So the test suite does normalize the expr IDs but misses some places?
There was a problem hiding this comment.
The (?<!id=) part is a negative lookbehind to not match id=#123 but match all other #123 like expr ids. id=#123 like ids are actually plan ids in SubqueryExec nodes.
The(?<prefix> capture group just simplifies replacement.
There was a problem hiding this comment.
The problem in some subset test runs was, that an id showed up both as plan id and as expression id. So the previous exprIdRegexp replaced all occurances to the same normalized one.
While in the golden run the ids were different so exprIdRegexp and planIdRegex replaced them to 2 different normalized ids.
There was a problem hiding this comment.
id=#123 like ids are actually plan ids in SubqueryExec nodes.
Ah I didn't know it. Can we leave a code comment to explain it?
There was a problem hiding this comment.
Sure, added in 7e1cbcf.
What changes were proposed in this pull request?
This PR fixes and id normalization in
PlanStabilitySuites andWhy are the changes needed?
Currently if we run a subset of
PlanStabilitySuites then it might fail due to plan and expression id conflixts:This is because the expr id regex doesn't skip and plan id regex doesn't take into account that the plan id is formatted as
id=#...inSubqueryExecnodes.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing UTs + manual subset run.
Was this patch authored or co-authored using generative AI tooling?
No.