[SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference #43966
Conversation
This file contains 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
|
cc. @cloud-fan PTAL, thanks! |
cloud-fan
reviewed
Nov 23, 2023
| statsOpt: Option[Statistics] = None) extends LeafNode with MultiInstanceRelation { | ||
|
|
||
| final override val nodePatterns: Seq[TreePattern] = Seq(CTE) | ||
|
|
||
| override lazy val resolved: Boolean = _resolved | ||
|
|
||
| override lazy val isStreaming: Boolean = _isStreaming |
There was a problem hiding this comment.
isStreaming is a def, so we can simply put override val isStreaming in the constructor
cloud-fan
approved these changes
Nov 23, 2023
cloud-fan
reviewed
Nov 23, 2023
...alyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
Outdated
Show resolved
Hide resolved
HeartSaVioR
force-pushed
the
SPARK-46062
branch
from
ec3c7c2
to
3ddeab3
Compare
…s/logical/basicLogicalOperators.scala Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>
HeartSaVioR
force-pushed
the
SPARK-46062
branch
from
4ace827
to
6840d25
Compare
|
https://github.com/HeartSaVioR/spark/actions/runs/6969118403/job/18967484716 It failed only from test_pandas_map with connection refused error message, which is unrelated. |
|
Thanks, merging to master/3.5/3.4. |
HeartSaVioR
added a commit
that referenced
this pull request
Nov 23, 2023
…nd reference This PR proposes to sync the flag `isStreaming` from CTE definition to CTE reference. The essential issue is that CTE reference node cannot determine the flag `isStreaming` by itself, and never be able to have a proper value and always takes the default as it does not have a parameter in constructor. The other flag `resolved` is handled, and we need to do the same for `isStreaming`. Once we add the parameter to the constructor, we will also need to make sure the flag is in sync with CTE definition. We have a rule `ResolveWithCTE` doing the sync, hence we add the logic to sync the flag `isStreaming` as well. The bug may impact some rules which behaves differently depending on isStreaming flag. It would no longer be a problem once CTE reference is replaced with CTE definition at some point in "optimization phase", but all rules in analyzer and optimizer being triggered before the rule takes effect may misbehave based on incorrect isStreaming flag. No. New UT. No. Closes #43966 from HeartSaVioR/SPARK-46062. Authored-by: Jungtaek Lim <kabhwan.opensource@gmail.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com> (cherry picked from commit 4304663) Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>
HeartSaVioR
added a commit
that referenced
this pull request
Nov 23, 2023
…nd reference This PR proposes to sync the flag `isStreaming` from CTE definition to CTE reference. The essential issue is that CTE reference node cannot determine the flag `isStreaming` by itself, and never be able to have a proper value and always takes the default as it does not have a parameter in constructor. The other flag `resolved` is handled, and we need to do the same for `isStreaming`. Once we add the parameter to the constructor, we will also need to make sure the flag is in sync with CTE definition. We have a rule `ResolveWithCTE` doing the sync, hence we add the logic to sync the flag `isStreaming` as well. The bug may impact some rules which behaves differently depending on isStreaming flag. It would no longer be a problem once CTE reference is replaced with CTE definition at some point in "optimization phase", but all rules in analyzer and optimizer being triggered before the rule takes effect may misbehave based on incorrect isStreaming flag. No. New UT. No. Closes #43966 from HeartSaVioR/SPARK-46062. Authored-by: Jungtaek Lim <kabhwan.opensource@gmail.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com> (cherry picked from commit 4304663) Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>
szehon-ho
pushed a commit
to szehon-ho/spark
that referenced
this pull request
Feb 7, 2024
…nd reference This PR proposes to sync the flag `isStreaming` from CTE definition to CTE reference. The essential issue is that CTE reference node cannot determine the flag `isStreaming` by itself, and never be able to have a proper value and always takes the default as it does not have a parameter in constructor. The other flag `resolved` is handled, and we need to do the same for `isStreaming`. Once we add the parameter to the constructor, we will also need to make sure the flag is in sync with CTE definition. We have a rule `ResolveWithCTE` doing the sync, hence we add the logic to sync the flag `isStreaming` as well. The bug may impact some rules which behaves differently depending on isStreaming flag. It would no longer be a problem once CTE reference is replaced with CTE definition at some point in "optimization phase", but all rules in analyzer and optimizer being triggered before the rule takes effect may misbehave based on incorrect isStreaming flag. No. New UT. No. Closes apache#43966 from HeartSaVioR/SPARK-46062. Authored-by: Jungtaek Lim <kabhwan.opensource@gmail.com> Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com> (cherry picked from commit 4304663) Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>
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 were proposed in this pull request?
This PR proposes to sync the flag
isStreamingfrom CTE definition to CTE reference.The essential issue is that CTE reference node cannot determine the flag
isStreamingby itself, and never be able to have a proper value and always takes the default as it does not have a parameter in constructor. The other flagresolvedis handled, and we need to do the same forisStreaming.Once we add the parameter to the constructor, we will also need to make sure the flag is in sync with CTE definition. We have a rule
ResolveWithCTEdoing the sync, hence we add the logic to sync the flagisStreamingas well.Why are the changes needed?
The bug may impact some rules which behaves differently depending on isStreaming flag. It would no longer be a problem once CTE reference is replaced with CTE definition at some point in "optimization phase", but all rules in analyzer and optimizer being triggered before the rule takes effect may misbehave based on incorrect isStreaming flag.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
New UT.
Was this patch authored or co-authored using generative AI tooling?
No.