[SPARK-21538][SQL] Attribute resolution inconsistency in the Dataset API #18740
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Test build #79973 has finished for PR 18740 at commit
|
|
Please add a test like #18744 did. Thanks. |
|
LGTM except for test. |
|
Thanks for fixing it! Please remember to add a test case whenever you fix anything else in the future. We really need to improve our test case coverage. BTW, welcome to contributing more test-only PRs. |
gatorsmile
reviewed
Jul 27, 2017
| checkAnswer(df.sort($"id"), df.sort("id")) | ||
| checkAnswer(df.sort('id), df.sort("id")) | ||
| checkAnswer(df.orderBy('id), df.sort("id")) | ||
| checkAnswer(df.orderBy("id"), df.sort("id")) |
There was a problem hiding this comment.
Since it sounds like you are very interested in Spark open source contribution, let me explain how we write the test cases. Normally, we do not want to duplicate the codes and also want to verify the results. Thus, maybe you can change the code like
val df = spark.range(3).withColumnRenamed("id", "x")
val expected = Row(0) :: Row(1) :: Row (2) :: Nil
checkAnswer(df.sort("id"), expected)
checkAnswer(df.sort(col("id")), expected)
checkAnswer(df.sort($"id"), expected)
checkAnswer(df.sort('id), expected)
checkAnswer(df.orderBy('id), expected)
checkAnswer(df.orderBy("id"), expected)There was a problem hiding this comment.
Indeed, looks much better. I appreciate the explanation and will take this into account in the future. I will update the test in a minute, thanks
|
Test build #80007 has finished for PR 18740 at commit
|
|
LGTM pending Jenkins |
|
Test build #80009 has finished for PR 18740 at commit
|
asfgit
pushed a commit
that referenced
this pull request
Jul 27, 2017
## What changes were proposed in this pull request?
This PR contains a tiny update that removes an attribute resolution inconsistency in the Dataset API. The following example is taken from the ticket description:
```
spark.range(1).withColumnRenamed("id", "x").sort(col("id")) // works
spark.range(1).withColumnRenamed("id", "x").sort($"id") // works
spark.range(1).withColumnRenamed("id", "x").sort('id) // works
spark.range(1).withColumnRenamed("id", "x").sort("id") // fails with:
org.apache.spark.sql.AnalysisException: Cannot resolve column name "id" among (x);
```
The above `AnalysisException` happens because the last case calls `Dataset.apply()` to convert strings into columns, which triggers attribute resolution. To make the API consistent between overloaded methods, this PR defers the resolution and constructs columns directly.
Author: aokolnychyi <anton.okolnychyi@sap.com>
Closes #18740 from aokolnychyi/spark-21538.
(cherry picked from commit f44ead8)
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
|
Thanks! Merging to master/2.2 |
MatthewRBruce
pushed a commit
to Shopify/spark
that referenced
this pull request
Jul 31, 2018
## What changes were proposed in this pull request?
This PR contains a tiny update that removes an attribute resolution inconsistency in the Dataset API. The following example is taken from the ticket description:
```
spark.range(1).withColumnRenamed("id", "x").sort(col("id")) // works
spark.range(1).withColumnRenamed("id", "x").sort($"id") // works
spark.range(1).withColumnRenamed("id", "x").sort('id) // works
spark.range(1).withColumnRenamed("id", "x").sort("id") // fails with:
org.apache.spark.sql.AnalysisException: Cannot resolve column name "id" among (x);
```
The above `AnalysisException` happens because the last case calls `Dataset.apply()` to convert strings into columns, which triggers attribute resolution. To make the API consistent between overloaded methods, this PR defers the resolution and constructs columns directly.
Author: aokolnychyi <anton.okolnychyi@sap.com>
Closes apache#18740 from aokolnychyi/spark-21538.
(cherry picked from commit f44ead8)
Signed-off-by: gatorsmile <gatorsmile@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR contains a tiny update that removes an attribute resolution inconsistency in the Dataset API. The following example is taken from the ticket description:
The above
AnalysisExceptionhappens because the last case callsDataset.apply()to convert strings into columns, which triggers attribute resolution. To make the API consistent between overloaded methods, this PR defers the resolution and constructs columns directly.