Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-12987][SQL]Fixing the name resolution in drop column #11009

Closed
wants to merge 2 commits into from

Conversation

kevinyu98
Copy link
Contributor

@marmbrus @cloud-fan @thomas @jayadevan : Hello All: Can you help review this code fix?

This problem is coming from drop column, after we drop off the old column, construct the new dataframe from the remaining columns.

the new dataframe is using the Schema information to construct the column name, in this case, the name is string ‘a.c’. Since it is from Schema, we should take the name as it is, we should not do any parsing on the name.

@marmbrus
Copy link
Contributor

marmbrus commented Feb 2, 2016

ok to test

@@ -1257,7 +1257,8 @@ class DataFrame private[sql](
def drop(colNames: String*): DataFrame = {
val resolver = sqlContext.analyzer.resolver
val remainingCols =
schema.filter(f => colNames.forall(n => !resolver(f.name, n))).map(f => Column(f.name))
schema.filter(f => colNames.forall(n => !resolver(f.name, n)))
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: i would consider breaking before the filter also if you have to break for the map. otherwise the filter looks special when really you are just chaining a bunch of operations.

@SparkQA
Copy link

SparkQA commented Feb 2, 2016

Test build #50515 has finished for PR 11009 at commit ff187c3.

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

@cloud-fan
Copy link
Contributor

is it duplicated with #10943?

@yzhou2001
Copy link
Contributor

The two jiras seem to be identical; but the fixes are different.

@dilipbiswal
Copy link
Contributor

@cloud-fan Yeah.. 10943 also fixes the same issue.

@yzhou2001 Actually the original fix was sort of similar to fix in this PR except that it also considered the drop(Column) interface. I have been trying to improve the fix based on suggestion from Wenchen :-)

@kevinyu98
Copy link
Contributor Author

@marmbrus @cloud-fan @dilipbiswal @yzhou2001 : it seems this is the duplicate of [SPARK-12988][SQL] Can't drop columns that contain dots #10943, I will close this PR. thanks.

@kevinyu98 kevinyu98 closed this Feb 2, 2016
@marmbrus
Copy link
Contributor

marmbrus commented Feb 2, 2016

Actually, I'd really like to include this in 1.6.1 and this is a nice small fix. Could you reopen?

@kevinyu98 kevinyu98 reopened this Feb 2, 2016
@kevinyu98
Copy link
Contributor Author

@marmbrus @cloud-fan @dilipbiswal @yzhou2001 : I have change the code based on Michael's comments, can you help review it again?

Not sure why the first test failed, I run the sql test locally, it passed.

[info] Run completed in 3 minutes, 23 seconds.
[info] Total number of tests run: 1553
[info] Suites: completed 110, aborted 0
[info] Tests: succeeded 1553, failed 0, canceled 0, ignored 10, pending 0
[info] All tests passed.
[info] Passed: Total 1553, Failed 0, Errors 0, Passed 1553, Ignored 10

@SparkQA
Copy link

SparkQA commented Feb 3, 2016

Test build #50686 has finished for PR 11009 at commit fb59a8a.

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

@rxin
Copy link
Contributor

rxin commented Jun 15, 2016

Thanks for the pull request. I'm going through a list of pull requests to cut them down since the sheer number is breaking some of the tooling we have. Due to lack of activity on this pull request, I'm going to push a commit to close it. Feel free to reopen it or create a new one. We can also continue the discussion on the JIRA ticket.

@asfgit asfgit closed this in 1a33f2e Jun 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants