-
Notifications
You must be signed in to change notification settings - Fork 972
[KYUUBI #5382][JDBC] Duplication cleanup improvement in JdbcDialect and schema helpers #5490
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
Conversation
...ls/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/schema/RowSetHelper.scala
Outdated
Show resolved
Hide resolved
...ls/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/dialect/JdbcDialect.scala
Outdated
Show resolved
Hide resolved
...yuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/JdbcOperation.scala
Outdated
Show resolved
Hide resolved
...yuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/operation/JdbcOperation.scala
Outdated
Show resolved
Hide resolved
...ls/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/schema/RowSetHelper.scala
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #5490 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 588 588
Lines 33466 33481 +15
Branches 4401 4405 +4
======================================
- Misses 33466 33481 +15 see 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@bowenliang123 hi, thanks for your suggestions, I have made all the changes. Please review it again when you have time :). |
bowenliang123
left a comment
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. Wait for @zhaomin1423 's review.
...ls/kyuubi-jdbc-engine/src/main/scala/org/apache/kyuubi/engine/jdbc/schema/RowSetHelper.scala
Outdated
Show resolved
Hide resolved
zhaomin1423
left a comment
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.
Thanks, LGTM
|
Thanks for the contribution from @zhuyaogai and the review by @zhaomin1423 . |
Why are the changes needed?
To close #5382.
How was this patch tested?
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
Was this patch authored or co-authored using generative AI tooling?
No