[CONNECT] Demonstrate stale cached plan without DSv2 refresh - #57683
[CONNECT] Demonstrate stale cached plan without DSv2 refresh#57683longvu-db wants to merge 2 commits into
Conversation
HyukjinKwon
left a comment
There was a problem hiding this comment.
1 blocking, 0 non-blocking, 0 nits.
Useful bug repro, but the production default flip is a global regression that must not merge; the targeted refresh gate belongs in the fix PR (#57609), not a flipped default here.
Design / architecture (1)
- sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala:73: This flips QueryExecution.refreshPhaseEnabled's primary-constructor default from true to false, which globally disables the DSv2/metadata refresh phase for every direct
new QueryExecution(the phase is gated atif (refreshPhaseEnabled)). That is the very stale-cached-plan regression the PR title demonstrates -- appropriate as a repro scaffold, but it must not merge as a production default. The real fix should gate refresh narrowly (as in the related DSv2-refresh-skip PR #57609) rather than turning it off everywhere. If this PR is only meant to demonstrate the bug, please mark it [WIP]/draft and keep the default at true (or drop the production change and rely on a test-local QueryExecution with refreshPhaseEnabled=false). -- see inline
Verification
Confirmed refreshPhaseEnabled gates the refresh phase (QueryExecution.scala:271 if (refreshPhaseEnabled)), the primary-ctor default is now false (line 73) while the helper factories still default true (lines 737, 947) -- so every direct new QueryExecution(...) now skips refresh. The added tests are faithful DSv2 stale-plan repros against an InMemoryTableCatalog. Build/test: build/sbt 'connect-server/testOnly *SparkConnectServerTestSuite'.
PR description suggestions
- The description is the empty template; state that this PR demonstrates the stale-cached-plan bug (and mark it [WIP]/draft), so reviewers do not read the refreshPhaseEnabled default flip as an intended production change.
| val mode: CommandExecutionMode.Value = CommandExecutionMode.ALL, | ||
| val shuffleCleanupModeOpt: Option[ShuffleCleanupMode] = None, | ||
| val refreshPhaseEnabled: Boolean = true, | ||
| val refreshPhaseEnabled: Boolean = false, |
There was a problem hiding this comment.
This flips the primary-constructor default of refreshPhaseEnabled from true to false, which globally disables the DSv2/metadata refresh phase for every direct new QueryExecution(...) (the phase is gated below at if (refreshPhaseEnabled)). That reintroduces the stale-cached-plan behavior this PR demonstrates, for all callers -- not just the Connect path. The helper factories still default to true, so this also makes direct construction diverge from them. If the intent is only to demonstrate the bug, please keep this default at true and drive the tests via a test-local QueryExecution with refreshPhaseEnabled = false (and mark the PR [WIP]/draft); the actual fix should gate refresh narrowly as in #57609 rather than turning it off by default.
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?