-
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-43920][SQL][CONNECT] Create sql/api module #41426
Conversation
<sbt.project.name>sql-api</sbt.project.name> | ||
</properties> | ||
|
||
<dependencies> |
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.
Should we add common/util as a dependency?
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.
I will add it once we move code into this module.
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.
LGTM
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.
sqlApi
module shouldn't participate in mima check in the current version
@LuciferYang right. I updated the SparkBuild to exclude it. |
Waiting CI |
Merging. |
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.
+1, LGTM.
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.
BTW, sorry for asking this question after merging.
- What is the minimal target for migration in order to help
connect
module? According to the PR description,Row
is also mentioned. So,sql/catalyst/src/main/scala/org/apache/spark/sql/types
is not enough? - Does it cause many MIMA exceptions against old
catalyst
module?
In current scope, Row, coders, data types are required. However we may discover more during the process. |
Got it. Let's see it in the actual PR~ |
### What changes were proposed in this pull request? We need a sql/api module to host public API like DataType, Row, etc. This module can be shared between Catalyst and Spark Connect client so that client do not need to depend on Catalyst anymore. ### Why are the changes needed? Towards Spark Connect client do not need to depend on Catalyst anymore. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? N/A Closes apache#41426 from amaliujia/add_sql_api. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Herman van Hovell <herman@databricks.com>
### What changes were proposed in this pull request? The pr aims to update doc `sql/README.md`. ### Why are the changes needed? After #41426, We have added a subproject `API` to our `SQL moudle`, so we need to update the doc `sql/README.md` synchronously. ### Does this PR introduce _any_ user-facing change? Yes, make the doc clearer and more accurate. ### How was this patch tested? Manually test. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #47476 from panbingkun/minor_docs. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
### What changes were proposed in this pull request? The pr aims to update doc `sql/README.md`. ### Why are the changes needed? After apache#41426, We have added a subproject `API` to our `SQL moudle`, so we need to update the doc `sql/README.md` synchronously. ### Does this PR introduce _any_ user-facing change? Yes, make the doc clearer and more accurate. ### How was this patch tested? Manually test. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47476 from panbingkun/minor_docs. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
### What changes were proposed in this pull request? The pr aims to update doc `sql/README.md`. ### Why are the changes needed? After apache#41426, We have added a subproject `API` to our `SQL moudle`, so we need to update the doc `sql/README.md` synchronously. ### Does this PR introduce _any_ user-facing change? Yes, make the doc clearer and more accurate. ### How was this patch tested? Manually test. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47476 from panbingkun/minor_docs. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
### What changes were proposed in this pull request? The pr aims to update doc `sql/README.md`. ### Why are the changes needed? After apache#41426, We have added a subproject `API` to our `SQL moudle`, so we need to update the doc `sql/README.md` synchronously. ### Does this PR introduce _any_ user-facing change? Yes, make the doc clearer and more accurate. ### How was this patch tested? Manually test. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47476 from panbingkun/minor_docs. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
What changes were proposed in this pull request?
We need a sql/api module to host public API like DataType, Row, etc. This module can be shared between Catalyst and Spark Connect client so that client do not need to depend on Catalyst anymore.
Why are the changes needed?
Towards Spark Connect client do not need to depend on Catalyst anymore.
Does this PR introduce any user-facing change?
No
How was this patch tested?
N/A