JDBC driver implements getDate, getTimestamp and getTime with Calendar in ResultSet #5752

zhaohehuhu · 2023-11-23T02:43:14Z

🔍 Description

Issue References 🔗

This pull request fixes #5650

Describe Your Solution 🔧

implemented getDate, getTimestamp and getTime with Calendar in Kyuubi JDBC driver side

Types of changes 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklists

📝 Author Self Checklist

My code follows the style guidelines of this project
I have performed a self-review
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
This patch was not authored or co-authored using Generative Tooling

📝 Committer Pre-Merge Checklist

Be nice. Be informative.

zhaohehuhu · 2023-11-23T02:48:03Z

@pan3793 plz review this feature. Thanks!

pan3793 · 2023-11-23T03:20:40Z

kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiArrowBasedResultSet.java

@@ -198,6 +199,32 @@ public Date getDate(String columnName) throws SQLException {
    return getDate(findColumn(columnName));
  }

+  public Date getDate(int columnIndex, Calendar cal) throws SQLException {


please add @Overwrite, and can we remove it from the parent abstract class?

…act class

codecov-commenter · 2023-11-23T05:02:57Z

Codecov Report

Attention: 80 lines in your changes are missing coverage. Please review.

Comparison is base (84a9686) 61.44% compared to head (36710d0) 61.22%.
Report is 39 commits behind head on master.

❗ Current head 36710d0 differs from pull request most recent head ddff6a7. Consider uploading reports for the commit ddff6a7 to get more accurate results

Files	Patch %	Lines
...he/kyuubi/jdbc/hive/KyuubiArrowBasedResultSet.java	0.00%	46 Missing ⚠️
...g/apache/kyuubi/jdbc/hive/KyuubiBaseResultSet.java	26.08%	30 Missing and 4 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5752      +/-   ##
============================================
- Coverage     61.44%   61.22%   -0.22%     
  Complexity       23       23              
============================================
  Files           607      607              
  Lines         35897    36030     +133     
  Branches       4923     4948      +25     
============================================
+ Hits          22058    22061       +3     
- Misses        11461    11571     +110     
- Partials       2378     2398      +20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…lendar

cfmcgrady · 2023-11-23T15:17:56Z

would you mind adding UTs within org.apache.kyuubi.operation.SparkDataTypeTests?

zhaohehuhu · 2023-11-24T01:34:55Z

would you mind adding UTs within org.apache.kyuubi.operation.SparkDataTypeTests?
Of course not. Let me take a look. Thanks!

zhaohehuhu · 2023-11-24T03:34:08Z

SparkDataTypeTests

added. plz review. Thanks.

cfmcgrady · 2023-11-24T03:47:45Z

cc @cxzl25

pan3793 · 2023-11-27T02:49:33Z

kyuubi-common/src/test/scala/org/apache/kyuubi/operation/SparkDataTypeTests.scala

+      assert(resultSet.next())
+      assert(resultSet.getTimestamp(
+        "col",
+        Calendar.getInstance(TimeZone.getTimeZone("UTC"))) === Timestamp.valueOf(


what if the TimeZone is not UTC?

Let me check it.

kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiBaseResultSet.java

kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiArrowBasedResultSet.java

…ubiBaseResultSet.java

…ubiArrowBasedResultSet.java

…me with Calendar in ResultSet # 🔍 Description ## Issue References 🔗 This pull request fixes #5650 ## Describe Your Solution 🔧 implemented getDate, getTimestamp and getTime with Calendar in Kyuubi JDBC driver side ## Types of changes 🔖 - [ ] Bugfix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [ ] Pull request title is okay. - [ ] No license issues. - [ ] Milestone correctly set? - [ ] Test coverage is ok - [ ] Assignees are selected. - [ ] Minimum number of approvals - [ ] No changes are requested **Be nice. Be informative.** Closes #5752 from zhaohehuhu/dev-1122. Closes #5752 ddff6a7 [Cheng Pan] Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiArrowBasedResultSet.java 595226d [Cheng Pan] Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiArrowBasedResultSet.java 8b386f1 [Cheng Pan] Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiArrowBasedResultSet.java 190c6b0 [Cheng Pan] Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiBaseResultSet.java dbc1e1e [Cheng Pan] Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiBaseResultSet.java e1a36d3 [Cheng Pan] Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/KyuubiBaseResultSet.java 36710d0 [hezhao2] refactor cd2e36f [hezhao2] add UTs in SparkDataTypeTests fe60822 [hezhao2] add UTs in SparkDataTypeTests 77313c2 [hezhao2] reformat code style 33e4305 [hezhao2] JDBC driver implements Result#getTimestamp and Result#getTime with Calendar 7623b85 [hezhao2] add Overwrite annotation and remove this method from the parent abstract class 757f037 [hezhao2] JDBC driver implements Result#getDate with Calendar Lead-authored-by: hezhao2 <hezhao2@cisco.com> Co-authored-by: Cheng Pan <pan3793@gmail.com> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit fcb41f4) Signed-off-by: Cheng Pan <chengpan@apache.org>

pan3793 · 2023-12-05T07:02:40Z

Thanks, merged to master/1.8

JDBC driver implements Result#getDate with Calendar

757f037

github-actions bot added module:hive module:ctl labels Nov 23, 2023

pan3793 reviewed Nov 23, 2023

View reviewed changes

add Overwrite annotation and remove this method from the parent abstr…

7623b85

…act class

zhaohehuhu added 2 commits November 23, 2023 14:25

JDBC driver implements Result#getTimestamp and Result#getTime with Ca…

33e4305

…lendar

reformat code style

77313c2

zhaohehuhu changed the title ~~JDBC driver implements Result#getDate with Calendar~~ JDBC driver implements Result#getDate#getTimestamp#getTime with Calendar Nov 23, 2023

zhaohehuhu changed the title ~~JDBC driver implements Result#getDate#getTimestamp#getTime with Calendar~~ JDBC driver implements getDate, getTimestamp and getTime with Calendar in ResultSet Nov 23, 2023

add UTs in SparkDataTypeTests

fe60822

github-actions bot added the module:common label Nov 24, 2023

add UTs in SparkDataTypeTests

cd2e36f

pan3793 reviewed Nov 27, 2023

View reviewed changes

refactor

36710d0

pan3793 reviewed Dec 5, 2023

View reviewed changes

pan3793 added 6 commits December 5, 2023 14:13

Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Kyu…

e1a36d3

…ubiBaseResultSet.java

Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Kyu…

dbc1e1e

…ubiBaseResultSet.java

Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Kyu…

190c6b0

…ubiBaseResultSet.java

Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Kyu…

8b386f1

…ubiArrowBasedResultSet.java

Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Kyu…

595226d

…ubiArrowBasedResultSet.java

Update kyuubi-hive-jdbc/src/main/java/org/apache/kyuubi/jdbc/hive/Kyu…

ddff6a7

…ubiArrowBasedResultSet.java

pan3793 approved these changes Dec 5, 2023

View reviewed changes

pan3793 assigned zhaohehuhu Dec 5, 2023

pan3793 added this to the v1.8.1 milestone Dec 5, 2023

pan3793 closed this in fcb41f4 Dec 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDBC driver implements getDate, getTimestamp and getTime with Calendar in ResultSet #5752

JDBC driver implements getDate, getTimestamp and getTime with Calendar in ResultSet #5752

zhaohehuhu commented Nov 23, 2023 •

edited

Loading

zhaohehuhu commented Nov 23, 2023

pan3793 Nov 23, 2023

codecov-commenter commented Nov 23, 2023 •

edited

Loading

cfmcgrady commented Nov 23, 2023

zhaohehuhu commented Nov 24, 2023

zhaohehuhu commented Nov 24, 2023

cfmcgrady commented Nov 24, 2023

pan3793 Nov 27, 2023

zhaohehuhu Nov 27, 2023

pan3793 commented Dec 5, 2023

JDBC driver implements getDate, getTimestamp and getTime with Calendar in ResultSet #5752

JDBC driver implements getDate, getTimestamp and getTime with Calendar in ResultSet #5752

Conversation

zhaohehuhu commented Nov 23, 2023 • edited Loading

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklists

📝 Author Self Checklist

📝 Committer Pre-Merge Checklist

zhaohehuhu commented Nov 23, 2023

pan3793 Nov 23, 2023

Choose a reason for hiding this comment

codecov-commenter commented Nov 23, 2023 • edited Loading

Codecov Report

cfmcgrady commented Nov 23, 2023

zhaohehuhu commented Nov 24, 2023

zhaohehuhu commented Nov 24, 2023

cfmcgrady commented Nov 24, 2023

pan3793 Nov 27, 2023

Choose a reason for hiding this comment

zhaohehuhu Nov 27, 2023

Choose a reason for hiding this comment

pan3793 commented Dec 5, 2023

zhaohehuhu commented Nov 23, 2023 •

edited

Loading

codecov-commenter commented Nov 23, 2023 •

edited

Loading