Skip to content
This repository has been archived by the owner on Sep 18, 2023. It is now read-only.

[NSE-984] fix concat_ws #992

Merged
merged 2 commits into from
Jun 28, 2022
Merged

[NSE-984] fix concat_ws #992

merged 2 commits into from
Jun 28, 2022

Conversation

zhouyuan
Copy link
Collaborator

What changes were proposed in this pull request?

This patch fixes concat_ws bug with only two params
Signed-off-by: Yuan Zhou [email protected]

How was this patch tested?

pass jenkins

@github-actions
Copy link

#984

new ColumnarConcatWs(exps, original)
if (cws.children.size < 3) {
// if there are only two params, should fallback to concat
new ColumnarConcat(exps, original)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks concatWs handles the cases with two or less inputs differently with concat in vanilla spark. So I guess we cannot convert it to columnarConcat for such cases. Right?

Here is the output of vanilla spark.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, let me fix this in the next commit

zhouyuan added 2 commits June 27, 2022 20:09
Signed-off-by: Yuan Zhou <[email protected]>
Signed-off-by: Yuan Zhou <[email protected]>
@@ -49,6 +49,16 @@ class ColumnarConcatWs(exps: Seq[Expression], original: Expression)
}

override def doColumnarCodeGen(args: java.lang.Object): (TreeNode, ArrowType) = {
if (exps.size == 2) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@PHILO-HE the logic has been moved here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good to me.

@zhouyuan zhouyuan merged commit 3bef6d4 into oap-project:main Jun 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants