You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mgaido91 <gi...@git.apache.org> on 2018/04/16 13:09:42 UTC
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
GitHub user mgaido91 opened a pull request:
https://github.com/apache/spark/pull/21080
[SPARK-23986][SQL] freshName can generate non-unique names
## 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
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mgaido91/spark SPARK-23986
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21080.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #21080
----
commit 8623918adf88609f8a4e0ad6b69ad5c1d54bdc86
Author: Marco Gaido <ma...@...>
Date: 2018-04-16T12:58:02Z
[SPARK-23986][SQL] freshName can generate non-unique names
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89448/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21080
**[Test build #89398 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89398/testReport)** for PR 21080 at commit [`8623918`](https://github.com/apache/spark/commit/8623918adf88609f8a4e0ad6b69ad5c1d54bdc86).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/21080#discussion_r181931283
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -575,7 +575,7 @@ class CodegenContext {
if (freshNameIds.contains(fullName)) {
val id = freshNameIds(fullName)
freshNameIds(fullName) = id + 1
- s"$fullName$id"
+ s"${fullName}_$id"
} else {
freshNameIds += fullName -> 1
fullName
--- End diff --
If the given name is something like `name1_1`, I think you can still produce non-unique variable name.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21080#discussion_r182028213
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -575,10 +575,10 @@ class CodegenContext {
if (freshNameIds.contains(fullName)) {
--- End diff --
now we don't need this if
```
val id = freshNameIds.getOrElse(fullName, 0)
val res = s"${fullName}_$id"
freshNameIds(fullName) = id + 1
res
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/2346/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/2387/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89455/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/21080
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21080
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/21080
cc @cloud-fan @kiszk @viirya
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21080
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89398/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21080#discussion_r181966326
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -575,7 +575,7 @@ class CodegenContext {
if (freshNameIds.contains(fullName)) {
val id = freshNameIds(fullName)
freshNameIds(fullName) = id + 1
- s"$fullName$id"
+ s"${fullName}_$id"
} else {
freshNameIds += fullName -> 1
fullName
--- End diff --
@viirya my comment was for the current code without your change.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by dzanozin <gi...@git.apache.org>.
Github user dzanozin commented on the issue:
https://github.com/apache/spark/pull/21080
Please ignore the error report, it was a mess in the machine configuration with some v2.3.0 leftovers. Works as expected after cleanup. Sorry!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/21080#discussion_r181931856
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -575,7 +575,7 @@ class CodegenContext {
if (freshNameIds.contains(fullName)) {
val id = freshNameIds(fullName)
freshNameIds(fullName) = id + 1
- s"$fullName$id"
+ s"${fullName}_$id"
} else {
freshNameIds += fullName -> 1
fullName
--- End diff --
I'd suggest something like `s"${fullName}_0"` at L581. It also solves the failed tests.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21080
**[Test build #89448 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89448/testReport)** for PR 21080 at commit [`0993827`](https://github.com/apache/spark/commit/099382769ca3b0f32028f479f6669bc9c93bd1f4).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/21080
@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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21080#discussion_r181957835
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -575,7 +575,7 @@ class CodegenContext {
if (freshNameIds.contains(fullName)) {
val id = freshNameIds(fullName)
freshNameIds(fullName) = id + 1
- s"$fullName$id"
+ s"${fullName}_$id"
} else {
freshNameIds += fullName -> 1
fullName
--- End diff --
Ah, sorry for my misunderstanding. To change line 581 would work well.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by dzanozin <gi...@git.apache.org>.
Github user dzanozin commented on the issue:
https://github.com/apache/spark/pull/21080
The code still generated method signatures like this in Spark v2.3.1 (and obviously fails because of `agg_expr_21` duplication):
```Java
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 {
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/2382/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/2392/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on the issue:
https://github.com/apache/spark/pull/21080
Hmmm... why is this failing? The change LGTM.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21080#discussion_r181952028
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -575,7 +575,7 @@ class CodegenContext {
if (freshNameIds.contains(fullName)) {
val id = freshNameIds(fullName)
freshNameIds(fullName) = id + 1
- s"$fullName$id"
+ s"${fullName}_$id"
} else {
freshNameIds += fullName -> 1
fullName
--- End diff --
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21080
**[Test build #89455 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89455/testReport)** for PR 21080 at commit [`0993827`](https://github.com/apache/spark/commit/099382769ca3b0f32028f479f6669bc9c93bd1f4).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21080
**[Test build #89398 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89398/testReport)** for PR 21080 at commit [`8623918`](https://github.com/apache/spark/commit/8623918adf88609f8a4e0ad6b69ad5c1d54bdc86).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/21080#discussion_r181950606
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -575,7 +575,7 @@ class CodegenContext {
if (freshNameIds.contains(fullName)) {
val id = freshNameIds(fullName)
freshNameIds(fullName) = id + 1
- s"$fullName$id"
+ s"${fullName}_$id"
} else {
freshNameIds += fullName -> 1
fullName
--- End diff --
Isn't changed to `s"${fullName}_$id"`? So you will get `a_0_1`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/89441/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/21080#discussion_r181953816
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -575,7 +575,7 @@ class CodegenContext {
if (freshNameIds.contains(fullName)) {
val id = freshNameIds(fullName)
freshNameIds(fullName) = id + 1
- s"$fullName$id"
+ s"${fullName}_$id"
} else {
freshNameIds += fullName -> 1
fullName
--- End diff --
Hmm, doesn't it be
`a_1` -> `a_1_0`
`a` -> `a_0`
`a` -> `a_1`
?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/21080
thanks, merging to master/2.3!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/21080
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21080
**[Test build #89455 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89455/testReport)** for PR 21080 at commit [`0993827`](https://github.com/apache/spark/commit/099382769ca3b0f32028f479f6669bc9c93bd1f4).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21080
**[Test build #89441 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89441/testReport)** for PR 21080 at commit [`58f72ae`](https://github.com/apache/spark/commit/58f72ae61a188b221cf6b99db566c9b428870cc5).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21080
**[Test build #89448 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89448/testReport)** for PR 21080 at commit [`0993827`](https://github.com/apache/spark/commit/099382769ca3b0f32028f479f6669bc9c93bd1f4).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21080
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21080: [SPARK-23986][SQL] freshName can generate non-uni...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21080#discussion_r181949672
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -575,7 +575,7 @@ class CodegenContext {
if (freshNameIds.contains(fullName)) {
val id = freshNameIds(fullName)
freshNameIds(fullName) = id + 1
- s"$fullName$id"
+ s"${fullName}_$id"
} else {
freshNameIds += fullName -> 1
fullName
--- End diff --
If I am correct, does this still have a conflict between `a_01` wth `a_0$id` where `$id = 1`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21080: [SPARK-23986][SQL] freshName can generate non-unique nam...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21080
**[Test build #89441 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/89441/testReport)** for PR 21080 at commit [`58f72ae`](https://github.com/apache/spark/commit/58f72ae61a188b221cf6b99db566c9b428870cc5).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org