Skip to content

Conversation

@cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

It's more clear to only do table lookup in ResolveTables rule (for v2 tables) and ResolveRelations rule (for v1 tables). ResolveInsertInto should only resolve the InsertIntoStatement with resolved relations.

Why are the changes needed?

to make the code simpler

Does this PR introduce any user-facing change?

no

How was this patch tested?

existing tests

}
v2TableOpt.map(DataSourceV2Relation.create).getOrElse(u)

case i @ InsertIntoStatement(u: UnresolvedRelation, _, _, _, _) if i.query.resolved =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

simpler to ResolveRelations, ResolveTables should handle both UnresolvedRelation and InsertIntoStatement(UnresolvedRelation, ...).

case _ =>
i
case i @ InsertIntoStatement(r: DataSourceV2Relation, _, _, _, _) if i.query.resolved =>
// ifPartitionNotExists is append with validation, but validation is not supported
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just indentation changes.

@cloud-fan
Copy link
Contributor Author

@SparkQA
Copy link

SparkQA commented Sep 12, 2019

Test build #110518 has finished for PR 25774 at commit 2753af5.

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

@maropu
Copy link
Member

maropu commented Sep 12, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Sep 13, 2019

Test build #110547 has finished for PR 25774 at commit 2753af5.

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

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM

@HyukjinKwon
Copy link
Member

Merged to master.

@rdblue
Copy link
Contributor

rdblue commented Sep 18, 2019

Late +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants