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-20245][SQL][minor] pass output to LogicalRelation directly #17552

Closed
wants to merge 2 commits into from

Conversation

cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

Currently LogicalRelation has a expectedOutputAttributes parameter, which makes it hard to reason about what the actual output is. Like other leaf nodes, LogicalRelation should also take output as a parameter, to simplify the logic

How was this patch tested?

existing tests

@cloud-fan
Copy link
Contributor Author

cc @gatorsmile

@@ -75,13 +75,13 @@ class PathOptionSuite extends DataSourceTest with SharedSQLContext {
|USING ${classOf[TestOptionsSource].getCanonicalName}
|OPTIONS (PATH '/tmp/path')
""".stripMargin)
assert(getPathOption("src") == Some("file:/tmp/path"))
assert(getPathOption("src").map(makeQualifiedPath) == Some(makeQualifiedPath("/tmp/path")))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes in this test suite are not related to this PR, just to reinforce it.

@SparkQA
Copy link

SparkQA commented Apr 6, 2017

Test build #75578 has finished for PR 17552 at commit 05a0904.

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

// exprId in `expectedOutputAttributes`.
// The reason is that, some relations(like parquet) will reconcile attribute names to
// workaround case insensitivity issue.
case (attr, expected) => attr.withExprId(expected.exprId)
Copy link
Member

Choose a reason for hiding this comment

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

It sounds like this logics mentioned in the comments is removed by this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! I found this logic is only useful when converting hive tables to data source tables, so I moved the logic there: https://github.com/apache/spark/pull/17552/files#diff-ee66e11b56c21364760a5ed2b783f863R215

Copy link
Member

Choose a reason for hiding this comment

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

Agree.

@gatorsmile
Copy link
Member

LGTM except only one comment

@gatorsmile
Copy link
Member

LGTM pending Jenkins.

@SparkQA
Copy link

SparkQA commented Apr 7, 2017

Test build #75587 has finished for PR 17552 at commit 0fbd4a6.

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

@cloud-fan
Copy link
Contributor Author

thanks for the review, merging to master!

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