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-32782][SS] Refactor StreamingRelationV2 and move it to catalyst #29633

Closed
wants to merge 1 commit into from

Conversation

xuanyuanking
Copy link
Member

What changes were proposed in this pull request?

Move StreamingRelationV2 to the catalyst module and bind with the Table interface.

Why are the changes needed?

Currently, the StreamingRelationV2 is bind with TableProvider. Since the V2 relation is not bound with DataSource, to make it more flexible and have better expansibility, it should be moved to the catalyst module and bound with the Table interface. We did a similar thing for DataSourceV2Relation.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing UT.

@HyukjinKwon HyukjinKwon changed the title [SPARK-32782][SS] Refactory StreamingRelationV2 and move it to catalyst [SPARK-32782][SS] Refactor StreamingRelationV2 and move it to catalyst Sep 3, 2020
@HeartSaVioR
Copy link
Contributor

I'm not clear about the policy - if we would like to ensure the logical plan is placed in catalyst module, then why not moving StreamingRelation as well? Does it couple with core module?

@xuanyuanking
Copy link
Member Author

Does it couple with core module?

@HeartSaVioR Yes, the StreamingRelation couples with DataSource. You may find all the DataSource resolving work is in FindDataSourceTable so it's ok to put it in the core module. But for v2 relation, it's unbound with the specific data source, both Table and TableProvider is in the catalyst, so move it can benefit for our future extension on StreamingRelationV2.

@cloud-fan
Copy link
Contributor

In general, the v1 plan should still be sql/core as v1 API is there. The v2 plan should be in catalyst as v2 API is there.

@HeartSaVioR
Copy link
Contributor

OK that sounds like the ideal place is in catalyst module, but we already put them to core module and they're exposed to the public already. Thanks for the explanation.

I wish we migrate to V2 sooner; looks like there're lots of TODO comments and built-in connectors look to be still V1.

Copy link
Contributor

@HeartSaVioR HeartSaVioR 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

@SparkQA
Copy link

SparkQA commented Sep 3, 2020

Test build #128226 has finished for PR 29633 at commit 1e24af5.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class StreamingRelationV2(

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