You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "HeartSaVioR (via GitHub)" <gi...@apache.org> on 2023/11/23 04:26:41 UTC
[PR] [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference [spark]
HeartSaVioR opened a new pull request, #43966:
URL: https://github.com/apache/spark/pull/43966
### What changes were proposed in this pull request?
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.
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #43966:
URL: https://github.com/apache/spark/pull/43966#discussion_r1402982898
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -862,12 +862,15 @@ case class CTERelationRef(
cteId: Long,
_resolved: Boolean,
override val output: Seq[Attribute],
+ _isStreaming: Boolean,
Review Comment:
Ah yes, should be better. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #43966:
URL: https://github.com/apache/spark/pull/43966#discussion_r1402981136
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -862,12 +862,15 @@ case class CTERelationRef(
cteId: Long,
_resolved: Boolean,
override val output: Seq[Attribute],
+ _isStreaming: Boolean,
Review Comment:
```suggestion
override val isStreaming: Boolean
```
this should work
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -862,12 +862,15 @@ case class CTERelationRef(
cteId: Long,
_resolved: Boolean,
override val output: Seq[Attribute],
+ _isStreaming: Boolean,
Review Comment:
```suggestion
override val isStreaming: Boolean,
```
this should work
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR closed pull request #43966: [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference
URL: https://github.com/apache/spark/pull/43966
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #43966:
URL: https://github.com/apache/spark/pull/43966#issuecomment-1823807380
cc. @cloud-fan PTAL, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #43966:
URL: https://github.com/apache/spark/pull/43966#issuecomment-1824430830
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #43966:
URL: https://github.com/apache/spark/pull/43966#issuecomment-1824278266
[29874fc](https://github.com/apache/spark/pull/43966/commits/29874fc07fe0a0272c94c27618062aca289ba72d)
Just fixed other part being broken from the suggestion.
[6840d25](https://github.com/apache/spark/pull/43966/commits/6840d253ed7d6df3f10af3bed91f46eb0ccf8565)
Just updated the golden files - the only diff is that CTE reference node now shows the value of isStreaming flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #43966:
URL: https://github.com/apache/spark/pull/43966#issuecomment-1824432139
Thanks, merging to master/3.5/3.4.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46062][SQL] Sync the isStreaming flag between CTE definition and reference [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #43966:
URL: https://github.com/apache/spark/pull/43966#discussion_r1402935565
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -862,12 +862,15 @@ case class CTERelationRef(
cteId: Long,
_resolved: Boolean,
override val output: Seq[Attribute],
+ _isStreaming: Boolean,
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
Review Comment:
`isStreaming` is a def, so we can simply put `override val isStreaming` in the constructor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org