Skip to content

Conversation

@cloud-fan
Copy link
Contributor

In SPARK-10743 we wrap cast with UnresolvedAlias to give Cast a better alias if possible. However, for cases like filter, the UnresolvedAlias can't be resolved and actually we don't need a better alias for this case. This PR move the cast wrapping logic to Column.named so that we will only do it when we need a alias name.

@cloud-fan
Copy link
Contributor Author

cc @yhuai @marmbrus

@cloud-fan
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Jan 16, 2016

Test build #49506 has finished for PR 10781 at commit aa3b4e8.

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

@cloud-fan cloud-fan changed the title [SPARK-12783][SQL] fix cast in filter [SPARK-12783][SQL][minor] fix cast in filter Jan 16, 2016
@cloud-fan cloud-fan changed the title [SPARK-12783][SQL][minor] fix cast in filter [SPARK-12783][SQL] fix cast in filter Jan 16, 2016
@cloud-fan cloud-fan changed the title [SPARK-12783][SQL] fix cast in filter [SPARK-12841][SQL] fix cast in filter Jan 16, 2016
Copy link
Contributor

Choose a reason for hiding this comment

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

We use transformUp at here to make this logic equivalent with the original logic that we have in the cast, right (since we want to fix it in 1.6)? (in future, will it be possible to just use cast Cast(ne: NamedExpression, to) => UnresolvedAlias(Cast(ne, to))?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, how about something like df.select($"a".cast("int").cast("string"))? Do we need to propagate the name to the top level?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, i see. It will be good to propagate the name to the top level. The current change can do the job, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, it works, I'll add a test for it.

@SparkQA
Copy link

SparkQA commented Jan 18, 2016

Test build #49604 has finished for PR 10781 at commit af96c5d.

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

@SparkQA
Copy link

SparkQA commented Jan 18, 2016

Test build #49612 has finished for PR 10781 at commit d8681b4.

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

@cloud-fan
Copy link
Contributor Author

ping @yhuai

@yhuai
Copy link
Contributor

yhuai commented Jan 18, 2016

LGTM. Merging to master and branch 1.6.

@asfgit asfgit closed this in 4f11e3f Jan 18, 2016
@cloud-fan cloud-fan deleted the bug branch January 18, 2016 22:17
@yhuai
Copy link
Contributor

yhuai commented Jan 18, 2016

there are a few conflicts with branch 1.6. Can you create a pr for the backport? Thanks!

@cloud-fan
Copy link
Contributor Author

yea, sure

asfgit pushed a commit that referenced this pull request Jan 19, 2016
In SPARK-10743 we wrap cast with `UnresolvedAlias` to give `Cast` a better alias if possible. However, for cases like filter, the `UnresolvedAlias` can't be resolved and actually we don't need a better alias for this case. This PR move the cast wrapping logic to `Column.named` so that we will only do it when we need a alias name.

backport #10781 to 1.6

Author: Wenchen Fan <[email protected]>

Closes #10819 from cloud-fan/bug.
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.

3 participants