You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/02/23 21:27:48 UTC
[GitHub] [spark] bersprockets opened a new pull request #35635: [SPARK-38308][SQL] Eagerly iterate over sequence of window expressions in `ExtractWindowExpressions`
bersprockets opened a new pull request #35635:
URL: https://github.com/apache/spark/pull/35635
### What changes were proposed in this pull request?
Pass an `IndexedSeq` (likely a `Vector`) to `ExtractWindowExpressions.extract` and `ExtractWindowExpressions.addWindow` rather than whatever sequence type was specified by the user (in the `Dataset.select` method).
To accomplish this, we only need to pass an `IndexedSeq` to `ExtractWindowExpressions.extract`. `ExtractWindowExpressions.extract` will then return another `IndexedSeq` that we will pass on to `ExtractWindowExpressions.addWindow`
### Why are the changes needed?
Consider this query:
```
val df = spark.range(0, 20).map { x =>
(x % 4, x + 1, x + 2)
}.toDF("a", "b", "c")
import org.apache.spark.sql.expressions._
val w = Window.partitionBy("a").orderBy("b")
val selectExprs = Stream(
sum("c").over(w.rowsBetween(Window.unboundedPreceding, Window.currentRow)).as("sumc"),
avg("c").over(w.rowsBetween(Window.unboundedPreceding, Window.currentRow)).as("avgc")
)
df.select(selectExprs: _*).show(false)
```
It fails with
```
org.apache.spark.sql.AnalysisException: Resolved attribute(s) avgc#23 missing from c#16L,a#14L,b#15L,sumc#21L in operator !Project [c#16L, a#14L, b#15L, sumc#21L, sumc#21L, avgc#23].;
```
If you change the Stream to a `Vector` (or even a `List`), it succeeds.
As with SPARK-38221, this is due to the use of this code pattern:
```
def someMethod (seq: Seq[xxx]) {
...
val outerDataStructure = <create outer data structure>
val newSeq = seq.map { x =>
...
code that puts something in outerDataStructure
...
}
...
code that uses outerDataStructure (and expects it to be populated)
...
}
```
If `seq` is a `Stream`, `seq.map` might be evaluated lazily, in which case `outerDataStructure` will not be fully populated before it is used.
Both `ExtractWindowExpressions.extract` and `ExtractWindowExpressions.addWindow` use this pattern, but the above example failure is due to the pattern's use in `ExtractWindowExpressions.addWindow` (`extractedWindowExprBuffer` does not get fully populated, so the Window operator does not produce the output expected by its parent projection).
I chose `IndexedSeq` not for its efficient indexing, but because `map` will eagerly iterate over it.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New unit test.
--
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
[GitHub] [spark] bersprockets commented on a change in pull request #35635: [SPARK-38308][SQL] Eagerly iterate over sequence of window expressions in `ExtractWindowExpressions`
Posted by GitBox <gi...@apache.org>.
bersprockets commented on a change in pull request #35635:
URL: https://github.com/apache/spark/pull/35635#discussion_r840971543
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -3141,7 +3141,7 @@ class Analyzer(override val catalogManager: CatalogManager)
// have been resolved.
case p @ Project(projectList, child)
if hasWindowFunction(projectList) && !p.expressions.exists(!_.resolved) =>
- val (windowExpressions, regularExpressions) = extract(projectList)
+ val (windowExpressions, regularExpressions) = extract(projectList.toIndexedSeq)
Review comment:
>shall we simply fix all the public APIs that take Seq, to always call toIndexedSeq
I thought about that, and I think we should do that as well.
Although right now, AFAIK, the only way to pass lazy data structures to these loops is through the public API, there are other paths to this code (e.g., the parser) and some future innocuous-seeming change might end up passing streams to these loops. For example, somebody might add code that uses `Javaconverters`:
```
scala> jal
res7: java.util.ArrayList[Integer] = [2]
scala> jal.iterator.asScala.toSeq
res8: Seq[Integer] = Stream(2, ?)
scala>
```
--
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
[GitHub] [spark] bersprockets commented on pull request #35635: [SPARK-38308][SQL] Eagerly iterate over sequence of window expressions in `ExtractWindowExpressions`
Posted by GitBox <gi...@apache.org>.
bersprockets commented on pull request #35635:
URL: https://github.com/apache/spark/pull/35635#issuecomment-1079396420
Gentlest of pings @cloud-fan @HyukjinKwon
--
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
[GitHub] [spark] HyukjinKwon edited a comment on pull request #35635: [SPARK-38308][SQL] Eagerly iterate over sequence of window expressions in `ExtractWindowExpressions`
Posted by GitBox <gi...@apache.org>.
HyukjinKwon edited a comment on pull request #35635:
URL: https://github.com/apache/spark/pull/35635#issuecomment-1079801876
@bersprockets feel free to backport to branch-3.2 and other branches if needed (I didn't cos it has a conflict).
--
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
[GitHub] [spark] HyukjinKwon commented on pull request #35635: [SPARK-38308][SQL] Eagerly iterate over sequence of window expressions in `ExtractWindowExpressions`
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35635:
URL: https://github.com/apache/spark/pull/35635#issuecomment-1079801876
@bersprockets feel free to backport to branch-3.2 (I didn't cos it has a conflict).
--
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
[GitHub] [spark] HyukjinKwon closed pull request #35635: [SPARK-38308][SQL] Eagerly iterate over sequence of window expressions in `ExtractWindowExpressions`
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #35635:
URL: https://github.com/apache/spark/pull/35635
--
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
[GitHub] [spark] HyukjinKwon commented on pull request #35635: [SPARK-38308][SQL] Eagerly iterate over sequence of window expressions in `ExtractWindowExpressions`
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #35635:
URL: https://github.com/apache/spark/pull/35635#issuecomment-1079801847
Merged to master and branch-3.3.
--
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
[GitHub] [spark] cloud-fan commented on a change in pull request #35635: [SPARK-38308][SQL] Eagerly iterate over sequence of window expressions in `ExtractWindowExpressions`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35635:
URL: https://github.com/apache/spark/pull/35635#discussion_r836062422
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
##########
@@ -3141,7 +3141,7 @@ class Analyzer(override val catalogManager: CatalogManager)
// have been resolved.
case p @ Project(projectList, child)
if hasWindowFunction(projectList) && !p.expressions.exists(!_.resolved) =>
- val (windowExpressions, regularExpressions) = extract(projectList)
+ val (windowExpressions, regularExpressions) = extract(projectList.toIndexedSeq)
Review comment:
Just an idea: instead of fixing the internal code here and there, shall we simply fix all the public APIs that take `Seq`, to always call `toIndexedSeq` so that the `Stream` is materialized?
--
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