You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhztheplayer (via GitHub)" <gi...@apache.org> on 2025/06/16 07:25:44 UTC
[PR] [SPARK-52484] Remove child.supportsColumnar assertion in ColumnarToRo… [spark]
zhztheplayer opened a new pull request, #51183:
URL: https://github.com/apache/spark/pull/51183
### What changes were proposed in this pull request?
The PR removes the unnecessary assertion in `ColumnarToRowExec` introduced by https://github.com/apache/spark/pull/25264 to guarantee some flexibilities for 3rd Spark plugins. Especially in Apache Gluten, the assertion blocks some of our effort in query optimization because we needed an intermediate state of the query plan which Spark may see as illegal.
Moreover, the reason why this intermediate state is needed in Gluten: Gluten has a cascades-style query optimizer (RAS) which could set a leaf, dummy, row-based plan node to hide up a sub-tree of the plan, to represent a so-called cascades 'group'. Although this pattern (C2R on a row-based plan) is illegal, it could be used as the input of an optimizer rule to potentially be used to convert into a valid query plan.
This change is to remove the assertion to ensure some flexibilities to the 3rd plugins. This should be no harm for the upstream Apache Spark, because the query execution will still be failed by [this error](https://github.com/apache/spark/blob/5d0b2f41794bf4dd25b3ce19bc4f634082b40876/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala#L343-L351) without this assertion on an illegal query plan.
### Does this PR introduce _any_ user-facing change?
Basically no. An assertion error in plan-building time will be replaced by an exception in execution time (still from the driver side) when an illegal query plan is set.
### How was this patch tested?
Existing UTs.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150675313
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
The need of clarification here might be related to the original writing of the code. could think about its equivalence:
```scala
if (!Utils.isInRunningSparkTask) {
// When we are on Spark driver, do the following assertion:
assert(child.supportsColumnar)
}
```
I was just to remove the inner assertion, so the whole if clause could be removed because of that.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn closed pull request #51183: [SPARK-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec
URL: https://github.com/apache/spark/pull/51183
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150631903
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
Put it back there simply.
--
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-52484][SQL] Remove child.supportsColumnar assertion in ColumnarToRowExec [spark]
Posted by "jinchengchenghh (via GitHub)" <gi...@apache.org>.
jinchengchenghh commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2149306802
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
Do you keep Utils.isInRunningSparkTask
--
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-52484][SQL] Remove child.supportsColumnar assertion in ColumnarToRowExec [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150583983
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
In that case, you should update the PR title, @zhztheplayer , in order to be consistent what you propose here. Otherwise, this PR cannot be merged because of the mismatch of the PR title and code.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150675313
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
The need of clarification here might be related to the original writing of the code. we can think about its equivalence:
```scala
if (!Utils.isInRunningSparkTask) {
// When we are on Spark driver, do the following assertion:
assert(child.supportsColumnar)
}
```
Once we are removing the inner assertion, we can just remove the whole if block instead.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150668829
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
Hi @dongjoon-hyun, I feel the change needs to be clarified a bit :)
The previous code was:
```scala
assert(Utils.isInRunningSparkTask || child.supportsColumnar)
```
The proposed change is to skip the `child.supportsColumnar` assertion from the driver side, so the whole assertion will actually become:
```scala
assert(Utils.isInRunningSparkTask || true)
```
then become:
```scala
assert(true)
```
Hence, the assertion can be completely removed.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150675313
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
The need of clarification here might be related to the original writing of the code. Could think about its equivalence:
```scala
if (!Utils.isInRunningSparkTask) {
// When we are on Spark driver, do the following assertion:
assert(child.supportsColumnar)
}
```
I was just to remove the inner assertion, so the whole if clause could be removed because of that.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on PR #51183:
URL: https://github.com/apache/spark/pull/51183#issuecomment-2983776966
Thanks everyone for reviewing!
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150622831
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
Thanks. Updated the PR title along with the JIRA title.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150675313
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
The need of clarification here might be related to the original writing of the code. Its equivalence is:
```scala
if (!Utils.isInRunningSparkTask) {
// When we are on Spark driver, do the following assertion:
assert(child.supportsColumnar)
}
```
I was just to remove the inner assertion, so the whole if clause could be removed because of that.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150630976
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
It seems that there is a misunderstanding, @zhztheplayer . I don't think the updated title is enough. To be clear, you should not remove `assert(Utils.isInRunningSparkTask)` part if the updated PR is what you want.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150675313
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
The need of clarification here might be related to the original writing of the code. we can think about its equivalence:
```scala
if (!Utils.isInRunningSparkTask) {
// When we are on Spark driver, do the following assertion:
assert(child.supportsColumnar)
}
```
I was just to remove the inner assertion, so the whole if clause could be removed because of that.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150668829
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
Hi @dongjoon-hyun, I feel the change needs to be clarified a bit :)
The previous code was:
```scala
assert(Utils.isInRunningSparkTask || child.supportsColumnar)
```
The proposed change is to skip the `child.supportsColumnar` assertion from the driver side, so the whole assertion will actually become:
```scala
assert(Utils.isInRunningSparkTask || true)
```
then become:
```scala
assert(true) // can be removed
```
--
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-52484][SQL] Remove child.supportsColumnar assertion in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2149321604
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
Short answer would be no... The story is that this in-task check was introduced in https://github.com/apache/spark/pull/35058 to skip the assertion on executor side to solve another issue. So this PR's change equates to skipping the assertion on driver side as well.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2151051028
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
Now, I got your point. Thank you for clarification. I misread the context, @zhztheplayer . :)
--
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-52484][SQL] Remove child.supportsColumnar assertion in ColumnarToRo… [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on PR #51183:
URL: https://github.com/apache/spark/pull/51183#issuecomment-2975436617
@cloud-fan @revans2
Would you mind taking a look? 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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #51183:
URL: https://github.com/apache/spark/pull/51183#discussion_r2150668829
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/Columnar.scala:
##########
@@ -66,9 +65,6 @@ trait ColumnarToRowTransition extends UnaryExecNode
* [[MapPartitionsInRWithArrowExec]]. Eventually this should replace those implementations.
*/
case class ColumnarToRowExec(child: SparkPlan) extends ColumnarToRowTransition with CodegenSupport {
- // supportsColumnar requires to be only called on driver side, see also SPARK-37779.
- assert(Utils.isInRunningSparkTask || child.supportsColumnar)
Review Comment:
Hi @dongjoon-hyun, I feel the change needs to be clarified a bit :)
The previous code was:
```scala
assert(Utils.isInRunningSparkTask || child.supportsColumnar)
```
The proposed change is to skip the `child.supportsColumnar` assertion from the driver side, so the whole assertion will actually become:
```scala
assert(Utils.isInRunningSparkTask || true)
```
then become:
```scala
assert(true)
```
Hence, I needed to completely remove the assertion.
--
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-52484][SQL] Skip child.supportsColumnar assertion from driver side in ColumnarToRowExec [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #51183:
URL: https://github.com/apache/spark/pull/51183#issuecomment-2982550632
Merged to master, thank you @zhztheplayer @dongjoon-hyun
--
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