-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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-17625] [SQL] set expectedOutputAttributes when converting SimpleCatalogRelation to LogicalRelation #15182
Conversation
Maybe the position of the test case is not right, where should I put it? |
Test build #65725 has finished for PR 15182 at commit
|
val plan = Dataset.ofRows(spark, Project(Seq(expr), relation)) | ||
plan.queryExecution.assertAnalyzed() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your test case can be simplified to
val tableName = "tbl"
withTable(tableName) {
spark.range(10).select('id as 'i, 'id as 'j).write.saveAsTable(tableName)
val relation = spark.sessionState.catalog.lookupRelation(TableIdentifier(tableName))
val expr = relation.resolve("i")
Dataset.ofRows(spark, Project(Seq(expr), relation))
}
The fix looks ok to me, but you need to improve your PR description. It might not be easy for reviewers to understand your fix based on your PR description. I do not know which test suite is the best place, but |
Let's put it in |
@cloud-fan Ok. |
LGTM pending Jenkins |
LGTM |
Test build #65756 has finished for PR 15182 at commit
|
thanks, merging to master! |
What changes were proposed in this pull request?
When converting
SimpleCatalogRelation
toLogicalRelation
, we also need to setexpectedOutputAttributes
, then the output inLogicalRelation
is the attributes inexpectedOutputAttributes
, otherwise, it will use its schema to generate new attributes (and new exprId's), so the output of LogicalRelation will be different from output of SimpleCatalogRelation.As a result, if we have a table in
InMemoryCatalog
, we can't lookup it (get aSimpleCatalogRelation
) and then use it to composeLogicalPlan
s.How was this patch tested?
add a test case