[SPARK-23986][SQL] freshName can generate non-unique names #21080

mgaido91 · 2018-04-16T13:09:38Z

What changes were proposed in this pull request?

We are using CodegenContext.freshName to get a unique name for any new variable we are adding. Unfortunately, this method currently fails to create a unique name when we request more than one instance of variables with starting name name1 and an instance with starting name name11.

The PR changes the way a new name is generated by CodegenContext.freshName so that we generate unique names in this scenario too.

How was this patch tested?

added UT

mgaido91 · 2018-04-16T13:09:55Z

cc @cloud-fan @kiszk @viirya

SparkQA · 2018-04-16T16:13:07Z

Test build #89398 has finished for PR 21080 at commit 8623918.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2018-04-16T20:48:16Z

Hmmm... why is this failing? The change LGTM.

viirya · 2018-04-17T01:37:50Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

+      s"${fullName}_$id"
    } else {
      freshNameIds += fullName -> 1
      fullName


If the given name is something like name1_1, I think you can still produce non-unique variable name.

I'd suggest something like s"${fullName}_0" at L581. It also solves the failed tests.

If I am correct, does this still have a conflict between a_01 wth a_0$id where $id = 1?

Isn't changed to s"${fullName}_$id"? So you will get a_0_1.

I think it still has a problem. for sequence a_1, a, a, we have duplicated name a_1.

We can solve this problem by always adding the postfix.

Hmm, doesn't it be

a_1 -> a_1_0
a -> a_0
a -> a_1

?

Ah, sorry for my misunderstanding. To change line 581 would work well with unlikely-used char.

@viirya my comment was for the current code without your change.

viirya · 2018-04-17T09:04:34Z

LGTM

cloud-fan · 2018-04-17T10:40:23Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

    } else {
      s"${freshNamePrefix}_$name"
    }
    if (freshNameIds.contains(fullName)) {


now we don't need this if

val id = freshNameIds.getOrElse(fullName, 0) val res = s"${fullName}_$id" freshNameIds(fullName) = id + 1 res

SparkQA · 2018-04-17T12:39:35Z

Test build #89441 has finished for PR 21080 at commit 58f72ae.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-04-17T12:40:22Z

Test build #89448 has finished for PR 21080 at commit 0993827.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mgaido91 · 2018-04-17T12:45:43Z

retest this please

cloud-fan · 2018-04-17T13:28:59Z

LGTM

SparkQA · 2018-04-17T16:24:28Z

Test build #89455 has finished for PR 21080 at commit 0993827.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2018-04-17T16:37:24Z

thanks, merging to master/2.3!

## What changes were proposed in this pull request? We are using `CodegenContext.freshName` to get a unique name for any new variable we are adding. Unfortunately, this method currently fails to create a unique name when we request more than one instance of variables with starting name `name1` and an instance with starting name `name11`. The PR changes the way a new name is generated by `CodegenContext.freshName` so that we generate unique names in this scenario too. ## How was this patch tested? added UT Author: Marco Gaido <[email protected]> Closes #21080 from mgaido91/SPARK-23986. (cherry picked from commit f39e82c) Signed-off-by: Wenchen Fan <[email protected]>

dzanozin · 2018-09-10T20:40:17Z

The code still generates method signatures like this in Spark v2.3.1 (and obviously fails because of agg_expr_21 duplication):

private void agg_doConsume1(byte agg_expr_01, boolean agg_exprIsNull_01,
                            short agg_expr_11, boolean agg_exprIsNull_11,
                            short agg_expr_21, boolean agg_exprIsNull_21,
                            int agg_expr_31, boolean agg_exprIsNull_31,
                            int agg_expr_41, boolean agg_exprIsNull_41,
                            int agg_expr_51, boolean agg_exprIsNull_51,
                            UTF8String agg_expr_61, boolean agg_exprIsNull_61,
                            byte agg_expr_71, boolean agg_exprIsNull_71,
                            long agg_expr_81, boolean agg_exprIsNull_81,
                            double agg_expr_91, boolean agg_exprIsNull_91,
                            long agg_expr_101, boolean agg_exprIsNull_101,
                            double agg_expr_111, boolean agg_exprIsNull_111,
                            long agg_expr_121, boolean agg_exprIsNull_121,
                            int agg_expr_131, boolean agg_exprIsNull_131,
                            long agg_expr_141, boolean agg_exprIsNull_141,
                            int agg_expr_151, boolean agg_exprIsNull_151,
                            boolean agg_expr_161, boolean agg_exprIsNull_161,
                            long agg_expr_171,
                            byte agg_expr_18, boolean agg_exprIsNull_18,
                            boolean agg_expr_19, boolean agg_exprIsNull_19,
                            byte agg_expr_20, boolean agg_exprIsNull_20,
                            boolean agg_expr_21, boolean agg_exprIsNull_21,
                            short agg_expr_22, boolean agg_exprIsNull_22,
                            int agg_expr_23, boolean agg_exprIsNull_23) throws java.io.IOException {

mgaido91 · 2018-09-11T08:40:52Z

@dzanozin the code you reported clearly misses this patch. Since I do see the commit on v2.3.1, I am a bit puzzled about the issue you reported. Please provide a reproducer so we can check whether this really affects 2.3.1 or higher versions.

dzanozin · 2018-09-11T10:34:30Z

Please ignore the error report, it was a mess in the machine configuration with some v2.3.0 debris. Works as expected after cleanup generating the following method signature:

private void agg_doConsume_1(byte agg_expr_0_1, boolean agg_exprIsNull_0_1,
                             short agg_expr_1_1, boolean agg_exprIsNull_1_1,
...

My apologies!

…"Redefinition of parameter") Ref: SPARK-23986 cherry picked from commit f39e82c We are using `CodegenContext.freshName` to get a unique name for any new variable we are adding. Unfortunately, this method currently fails to create a unique name when we request more than one instance of variables with starting name `name1` and an instance with starting name `name11`. The PR changes the way a new name is generated by `CodegenContext.freshName` so that we generate unique names in this scenario too. added UT Author: Marco Gaido <[email protected]> Closes apache#21080 from mgaido91/SPARK-23986. (cherry picked from commit f39e82c)

We are using `CodegenContext.freshName` to get a unique name for any new variable we are adding. Unfortunately, this method currently fails to create a unique name when we request more than one instance of variables with starting name `name1` and an instance with starting name `name11`. The PR changes the way a new name is generated by `CodegenContext.freshName` so that we generate unique names in this scenario too. added UT Author: Marco Gaido <[email protected]> Closes apache#21080 from mgaido91/SPARK-23986. (cherry picked from commit f39e82c) Signed-off-by: Wenchen Fan <[email protected]> Ref: LIHADOOP-56518 (cherry picked from 564019b) RB=2368692 BUG=LIHADOOP-56518 G=spark-reviewers R=rhu,mshen A=mmuralid,yezhou,mshen

[SPARK-23986][SQL] freshName can generate non-unique names

8623918

viirya reviewed Apr 17, 2018

View reviewed changes

fix a and a_1 conflict

58f72ae

cloud-fan reviewed Apr 17, 2018

View reviewed changes

review comments

0993827

asfgit closed this in f39e82c Apr 17, 2018

[SPARK-23986][SQL] freshName can generate non-unique names #21080

[SPARK-23986][SQL] freshName can generate non-unique names #21080

Uh oh!

Conversation

mgaido91 commented Apr 16, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

mgaido91 commented Apr 16, 2018

Uh oh!

SparkQA commented Apr 16, 2018

Uh oh!

hvanhovell commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 17, 2018

Uh oh!

SparkQA commented Apr 17, 2018

Uh oh!

mgaido91 commented Apr 17, 2018

Uh oh!

cloud-fan commented Apr 17, 2018

Uh oh!

SparkQA commented Apr 17, 2018

Uh oh!

cloud-fan commented Apr 17, 2018

Uh oh!

dzanozin commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgaido91 commented Sep 11, 2018

Uh oh!

dzanozin commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kiszk Apr 17, 2018 •

edited

Loading

dzanozin commented Sep 10, 2018 •

edited

Loading

dzanozin commented Sep 11, 2018 •

edited

Loading