-
Notifications
You must be signed in to change notification settings - Fork 919
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
[KYUUBI #307]GetCatalogs supports DSv2 and keeps its backward compatibility #307
Conversation
...i-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/GetCatalogs.scala
Outdated
Show resolved
Hide resolved
...spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/GetTableTypes.scala
Show resolved
Hide resolved
Is it possible that create a util class for each in-compatible method with different spark version. For example: source code path: /v3.0/src/main
|
Codecov Report
@@ Coverage Diff @@
## master #307 +/- ##
==========================================
- Coverage 79.62% 79.51% -0.12%
==========================================
Files 95 98 +3
Lines 3402 3432 +30
Branches 399 404 +5
==========================================
+ Hits 2709 2729 +20
- Misses 493 497 +4
- Partials 200 206 +6
|
I recommend introduce a |
I prefer this way |
...s/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/shim/Shim_v3_0.scala
Show resolved
Hide resolved
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.
Scan Summary
Tool | Critical | High | Medium | Low | Status |
---|---|---|---|---|---|
Dependency Scan (java) | 0 | 2 | 5 | 0 | ✅ |
Security Audit for Infrastructure | 0 | 0 | 0 | 0 | ✅ |
Class File Analyzer | 0 | 6 | 0 | 0 | ❌ |
Scala Security Audit | 0 | 6 | 0 | 0 | ❌ |
Shell Script Analysis | 0 | 0 | 0 | 0 | ✅ |
Java Source Analyzer | 0 | 0 | 0 | 1 | ✅ |
Recommendation
Please review the findings from Code scanning alerts before approving this pull request. You can also configure the build rules or add suppressions to customize this bot 👍
I add a screenshot in the PR desc, is that what you expected @pan3793 |
|
||
class Shim_v2_4 extends SparkShim { | ||
override def getCatalogs(ss: SparkSession): Seq[Row] = { | ||
Seq(Row("spark_catalog")) |
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.
Does Spark v2.4 have the concept of catalog? Maybe here we shall return ""
, anyway we can check this later when supporting Spark v2.4
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.
Scan Summary
Tool | Critical | High | Medium | Low | Status |
---|---|---|---|---|---|
Dependency Scan (java) | 0 | 2 | 5 | 0 | ✅ |
Shell Script Analysis | 0 | 0 | 0 | 0 | ✅ |
Class File Analyzer | 0 | 6 | 0 | 0 | ❌ |
Security Audit for Infrastructure | 0 | 0 | 0 | 0 | ✅ |
Java Source Analyzer | 0 | 0 | 0 | 1 | ✅ |
Scala Security Audit | 0 | 6 | 0 | 0 | ❌ |
Recommendation
Please review the findings from Code scanning alerts before approving this pull request. You can also configure the build rules or add suppressions to customize this bot 👍
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 add a screenshot in the PR desc, is that what you expected @pan3793
Nice! LGTM, just few comments.
❨?❩
Please add issue ID here?
Fixes #307
Why are the changes needed?
GetCatalogs supports DSv2 and keeps its backward compatibility
Test Plan:
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request