Skip to content

[GLUTEN-8414][VL] Override doCanonicalize in ColumnarPartialProjectEx…#8415

Merged
zhztheplayer merged 4 commits intoapache:mainfrom
lifulong:canonicalize-columnar-partial-project-node
Jan 9, 2025
Merged

[GLUTEN-8414][VL] Override doCanonicalize in ColumnarPartialProjectEx…#8415
zhztheplayer merged 4 commits intoapache:mainfrom
lifulong:canonicalize-columnar-partial-project-node

Conversation

@lifulong
Copy link
Copy Markdown
Contributor

@lifulong lifulong commented Jan 3, 2025

…ec node

What changes were proposed in this pull request?

implement doCanonicalize in ColumnarPartialProjectExec node, for reuse exchange contains ColumnarPartialProjectExec
(same kind problem as #3154)
(Fixes: #8414)

How was this patch tested?

manual test in our product env

image

the biggest table read four times before implement doCanonicalize in ColumnarPartialProjectExec

image

the biggest table read one times before implement doCanonicalize in ColumnarPartialProjectExec

image

we can also see the stage reuse multi times after implement doCanonicalize in ColumnarPartialProjectExec in spark plan

@github-actions github-actions bot added the VELOX label Jan 3, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 3, 2025

#8414

@FelixYBW
Copy link
Copy Markdown
Contributor

FelixYBW commented Jan 3, 2025

Thank you for your first PR!

@zhztheplayer
Copy link
Copy Markdown
Member

Thanks for the contribution in advance.

I am also curious why replacedAliasUdf was placed into 2nd parameter list of the class? Which might be related to the issue per my understanding? cc @jinchengchenghh

https://github.com/apache/incubator-gluten/blob/4867d60bcde6627abb6f6a0988772b069f1229be/backends-velox/src/main/scala/org/apache/gluten/execution/ColumnarPartialProjectExec.scala#L53-L54

@jinchengchenghh
Copy link
Copy Markdown
Contributor

Because the class Alias ExprId is in the 2rd parameters, I would not like to change it when copy. @zhztheplayer

case class Alias(child: Expression, name: String)(
    val exprId: ExprId = NamedExpression.newExprId,
    val qualifier: Seq[String] = Seq.empty,
    val explicitMetadata: Option[Metadata] = None,
    val nonInheritableMetadataKeys: Seq[String] = Seq.empty)

@lifulong lifulong force-pushed the canonicalize-columnar-partial-project-node branch from 773fa10 to 54d0a6f Compare January 6, 2025 02:25
@lifulong
Copy link
Copy Markdown
Contributor Author

lifulong commented Jan 9, 2025

@zhztheplayer @jinchengchenghh hello, is this pr ready for merging ?

Copy link
Copy Markdown
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@zhztheplayer zhztheplayer merged commit 8109665 into apache:main Jan 9, 2025
zml1206 pushed a commit to zml1206/gluten that referenced this pull request Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ColumnarPartialProjectExec has special filed but not implement doCanonicalize method

5 participants