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-13282][SQL] LogicalPlan toSql should just return a String #11171

Closed
wants to merge 2 commits into from

Conversation

rxin
Copy link
Contributor

@rxin rxin commented Feb 11, 2016

Previously we were using Option[String] and None to indicate the case when Spark fails to generate SQL. It is easier to just use exceptions to propagate error cases, rather than having for comprehension everywhere. I also introduced a "build" function that simplifies string concatenation (i.e. no need to reason about whether we have an extra space or not).

@rxin
Copy link
Contributor Author

rxin commented Feb 11, 2016

cc @liancheng

@SparkQA
Copy link

SparkQA commented Feb 11, 2016

Test build #51093 has finished for PR 11171 at commit 9fd34fc.

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


case _ => None
case _ =>
throw new UnsupportedOperationException("")
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not using empty error message here?

@rxin
Copy link
Contributor Author

rxin commented Feb 12, 2016

Updated.

cc @gatorsmile unfortunately you will have to rebase your pull requests, although it should be easy to do.

@SparkQA
Copy link

SparkQA commented Feb 12, 2016

Test build #51178 has finished for PR 11171 at commit cfccf21.

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

@gatorsmile
Copy link
Member

It is easy to rebase. After the merge, I will submit PRs for the other related JIRAs. Thanks!

@rxin
Copy link
Contributor Author

rxin commented Feb 12, 2016

Thanks - I've merged this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants