You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "baibaichen (via GitHub)" <gi...@apache.org> on 2024/08/07 09:34:35 UTC
[PR] [GLUTEN-6705] [CORE] [Part 2] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
baibaichen opened a new pull request, #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745
## What changes were proposed in this pull request?
This PR avoids such transition by returning `Convention` which ouputs both `VanillaRow` and `BatchType`.
```scala
case _: ColumnarWriteFilesExec.ExecuteWriteCompatible =>
// ColumnarWriteFilesExec neither output Row nor columnar data.
// We output both row and columnar to avoid c2r and r2c transitions.
Convention.of(
Convention.RowType.VanillaRow,
BackendsApiManager.getSparkPlanExecApiInstance.batchType)
```
### why need
`ColumnarWriteFilesExec` neither outputs Columnar batch data nor `InternalRow`, and hence we can't add c2r or r2c on top of it, otherwise it would be am internal error.
1. In the normal code path, `DataWritingCommandExec` is on top of `ColumnarWriteFilesExec` and `DataWritingCommandExec`'s Convention is `VanillaRow` which avoid c2r transition.
2. In ClickHouse Backend, we are preparing to support native delta write in Spark 3.5 and delta 3.2, `WriteFile` is directly added on top of delta logical plan and hence the root `SparkPlan` is `ColumnarWriteFilesExec` which casue internal error without this change.
(Fixes: \#6705)
## How was this patch tested?
Existed 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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6705] [CORE] [Part 1] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745#issuecomment-2274916554
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6705] [CORE] [Part 1] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745#discussion_r1708207740
##########
gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transitions.scala:
##########
@@ -107,8 +100,13 @@ object Transitions {
}
private def toBatchPlan(plan: SparkPlan, toBatchType: Convention.BatchType): SparkPlan = {
+ convert(
+ plan,
+ ConventionReq.of(ConventionReq.RowType.Any, ConventionReq.BatchType.Is(toBatchType)))
+ }
+
+ private def convert(plan: SparkPlan, req: ConventionReq): SparkPlan = {
Review Comment:
I'd suggest to rename `convert` which sounds too general. How about `enforceReq`?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6705] [CORE] [Part 1] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745#discussion_r1708181313
##########
gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/ConventionFunc.scala:
##########
@@ -92,6 +92,12 @@ object ConventionFunc {
}
val conv = Convention.of(rowType, batchType)
conv
+ case _: ColumnarWriteFilesExec.ExecuteWriteCompatible =>
+ // ColumnarWriteFilesExec neither output Row nor columnar data.
+ // We output both row and columnar to avoid c2r and r2c transitions.
+ Convention.of(
+ Convention.RowType.VanillaRow,
+ BackendsApiManager.getSparkPlanExecApiInstance.batchType)
Review Comment:
Basically this list is for overriding conventions of vanilla Spark's operators. Would you use interface `KnowBatchType` / `KnownRowType` instead?
##########
gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/ConventionFunc.scala:
##########
@@ -156,6 +158,10 @@ object ConventionFunc {
case write: DataWritingCommandExec if SparkShimLoader.getSparkShims.isPlannedV1Write(write) =>
// To align with ApplyColumnarRulesAndInsertTransitions#insertTransitions
ConventionReq.any
+ case _: ColumnarWriteFilesExec.ExecuteWriteCompatible =>
+ ConventionReq.of(
+ ConventionReq.RowType.Any,
+ ConventionReq.BatchType.Is(BackendsApiManager.getSparkPlanExecApiInstance.batchType))
Review Comment:
The same. Use interface `KnownChildrenConventions` instead.
##########
gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transitions.scala:
##########
@@ -107,8 +100,13 @@ object Transitions {
}
private def toBatchPlan(plan: SparkPlan, toBatchType: Convention.BatchType): SparkPlan = {
+ convert(
+ plan,
+ ConventionReq.of(ConventionReq.RowType.Any, ConventionReq.BatchType.Is(toBatchType)))
+ }
+
+ private def convert(plan: SparkPlan, req: ConventionReq): SparkPlan = {
Review Comment:
I'd suggest rename `convert` which sounds too general. How about ``enforceReq`?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6705] [CORE] [Part 1] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on PR #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745#issuecomment-2273420172
cc @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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6705] [CORE] [Part 2] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745#issuecomment-2273043484
Run Gluten Clickhouse CI
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6705] [CORE] [Part 2] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745#issuecomment-2273043075
https://github.com/apache/incubator-gluten/issues/6705
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6705] [CORE] [Part 1] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745#discussion_r1708207740
##########
gluten-core/src/main/scala/org/apache/gluten/extension/columnar/transition/Transitions.scala:
##########
@@ -107,8 +100,13 @@ object Transitions {
}
private def toBatchPlan(plan: SparkPlan, toBatchType: Convention.BatchType): SparkPlan = {
+ convert(
+ plan,
+ ConventionReq.of(ConventionReq.RowType.Any, ConventionReq.BatchType.Is(toBatchType)))
+ }
+
+ private def convert(plan: SparkPlan, req: ConventionReq): SparkPlan = {
Review Comment:
I'd suggest rename `convert` which sounds too general. How about `enforceReq`?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6705] [CORE] [Part 1] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
Posted by "FelixYBW (via GitHub)" <gi...@apache.org>.
FelixYBW commented on PR #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745#issuecomment-2274517431
@JkSelf
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-6705] [CORE] [Part 1] Avoid adding c2r for ColumnarWriteFilesExec, since it neither output Columnar batch data nor InternalRow [incubator-gluten]
Posted by "baibaichen (via GitHub)" <gi...@apache.org>.
baibaichen merged PR #6745:
URL: https://github.com/apache/incubator-gluten/pull/6745
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org