[DRAFT] [JDBC] Fix `ResultSet.getTime()` #192

margarit-h · 2022-12-16T22:41:50Z

Signed-off-by: Margarit Hakobyan [email protected]

Description

ResultSet.getTime() returns non-null value reflecting the retrieved time instead of null.
PR in sql-jdbc repo https://github.com/Bit-Quill/sql-jdbc/pull/1/files

Issues Resolved

opensearch-project/sql-jdbc#20

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2022-12-16T22:51:17Z

Codecov Report

Merging #192 (aad073b) into integ-fix-resultset-get-time (18661b4) will decrease coverage by 2.48%.
The diff coverage is n/a.

@@                        Coverage Diff                         @@
##             integ-fix-resultset-get-time     #192      +/-   ##
==================================================================
- Coverage                           98.32%   95.83%   -2.49%     
  Complexity                           3552     3552              
==================================================================
  Files                                 346      356      +10     
  Lines                                8756     9414     +658     
  Branches                              554      673     +119     
==================================================================
+ Hits                                 8609     9022     +413     
- Misses                                142      334     +192     
- Partials                                5       58      +53

Flag	Coverage Δ
query-workbench	`62.76% <ø> (?)`
sql-engine	`98.32% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
workbench/public/components/Header/Header.tsx	`100.00% <0.00%> (ø)`
...ublic/components/QueryResults/QueryResultsBody.tsx	`68.32% <0.00%> (ø)`
workbench/public/application.tsx	`0.00% <0.00%> (ø)`
workbench/public/components/SQLPage/SQLPage.tsx	`100.00% <0.00%> (ø)`
workbench/public/utils/PanelWrapper.tsx	`100.00% <0.00%> (ø)`
...ch/public/components/QueryResults/QueryResults.tsx	`61.60% <0.00%> (ø)`
...h/public/components/QueryLanguageSwitch/Switch.tsx	`85.71% <0.00%> (ø)`
workbench/public/components/app.tsx	`0.00% <0.00%> (ø)`
workbench/public/components/PPLPage/PPLPage.tsx	`56.52% <0.00%> (ø)`
workbench/public/components/Main/main.tsx	`53.00% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sql-jdbc/src/main/java/org/opensearch/jdbc/types/TimeType.java

Signed-off-by: Margarit Hakobyan <[email protected]>

sql-jdbc/src/main/java/org/opensearch/jdbc/types/TimeType.java

acarbonetto · 2022-12-29T21:50:48Z

sql-jdbc/src/main/java/org/opensearch/jdbc/ResultSetImpl.java

+        log.debug(() -> logEntry("getTime (%d, %s)", columnIndex,
+                cal == null ? "null" : "Calendar TZ= " + cal.getTimeZone()));
+        checkCursorOperationPossible();
+        Time value = getTimeX(columnIndex, cal);


this will get called with or without debug. Can we put this in the log.debug() statement below?

log.debug(() -> logExit("getTime", getTimeX(columnIndex, cal)));

acarbonetto · 2022-12-29T21:50:59Z

sql-jdbc/src/main/java/org/opensearch/jdbc/ResultSetImpl.java

+                cal == null ? "null" : "Calendar TZ= " + cal.getTimeZone()));
+        checkCursorOperationPossible();
+        Time value = getTimeX(getColumnIndex(columnLabel), cal);
+        log.debug(() -> logExit("getTime", value));


acarbonetto · 2022-12-29T21:52:37Z

sql-jdbc/src/main/java/org/opensearch/jdbc/types/TimeType.java

-  public Time asTime(String value) throws SQLException {
-    return localTimetoSqlTime(toLocalTime(value));
-  }
+    if (value.length() > 10) {


instead of checking for length, can we try..catch an error from DateTimeFormatter if the pattern doesn't match?

acarbonetto · 2023-01-03T18:59:24Z

related: opensearch-project/sql-jdbc#20

Signed-off-by: Margarit Hakobyan <[email protected]>

acarbonetto · 2023-01-03T19:27:28Z

sql-jdbc/src/main/java/org/opensearch/jdbc/ResultSetImpl.java

+        checkCursorOperationPossible();
+        log.debug(() -> {
+            try {
+                return logExit("getTime", getTimeX(columnIndex, cal));


Whoops. This is not right. I misunderstood, and thought getTimeX was only needed for debug calls... but we certainly don't want this. We should only call getTimeX once. Please revert this back to what you had before.

acarbonetto · 2023-01-03T19:27:46Z

sql-jdbc/src/main/java/org/opensearch/jdbc/ResultSetImpl.java

+        checkCursorOperationPossible();
+        log.debug(() -> {
+            try {
+                return logExit("getTime", getTimeX(getColumnIndex(columnLabel), cal));


same here. We really should only call getTimeX once.

Signed-off-by: Margarit Hakobyan <[email protected]>

Yury-Fridlyand · 2023-01-04T00:42:55Z

sql-jdbc/src/main/java/org/opensearch/jdbc/types/TimeType.java

+    } catch (DateTimeParseException exception) {
+      time = Time.valueOf(value);
+    }


When it can fallback to catch block? Won't it throw an exception from there if value is not convertable to Time?

margarit-h · 2023-01-05T21:11:09Z

An upstream PR in sql-jdbc repo got merged. opensearch-project/sql-jdbc#41

margarit-h changed the title ~~Fix Result.Set.getTime()~~ [WIP:] Fix Result.Set.getTime() Dec 16, 2022

margarit-h changed the title ~~[WIP:] Fix Result.Set.getTime()~~ [WIP:] Fix ResultSet.getTime() Dec 16, 2022

margarit-h changed the title ~~[WIP:] Fix ResultSet.getTime()~~ [WIP:] [JDBC] Fix ResultSet.getTime() Dec 16, 2022

margarit-h marked this pull request as draft December 16, 2022 23:02

acarbonetto reviewed Dec 19, 2022

View reviewed changes

sql-jdbc/src/main/java/org/opensearch/jdbc/types/TimeType.java Outdated Show resolved Hide resolved

margarit-h added 6 commits December 22, 2022 15:52

Fix ResultSet.getTime()

40e3cd2

Signed-off-by: Margarit Hakobyan <[email protected]>

Fix a test

85cb03f

Signed-off-by: Margarit Hakobyan <[email protected]>

Fix tests

827763f

Signed-off-by: Margarit Hakobyan <[email protected]>

Refactoring

69c03db

Signed-off-by: Margarit Hakobyan <[email protected]>

Reverse unnecessary changes

d66627a

Signed-off-by: Margarit Hakobyan <[email protected]>

Refactor, add unit tests

8d15c35

Signed-off-by: Margarit Hakobyan <[email protected]>

margarit-h force-pushed the dev-fix-resultset-get-time branch from 8f8eebb to 8d15c35 Compare December 22, 2022 23:54

GabeFernandez310 reviewed Dec 28, 2022

View reviewed changes

sql-jdbc/src/main/java/org/opensearch/jdbc/types/TimeType.java Show resolved Hide resolved

acarbonetto approved these changes Dec 29, 2022

View reviewed changes

GabeFernandez310 approved these changes Dec 29, 2022

View reviewed changes

Address PR review feedback

c217ce2

Signed-off-by: Margarit Hakobyan <[email protected]>

acarbonetto reviewed Jan 3, 2023

View reviewed changes

Reverted ResultSetImpl latest changes

aad073b

Signed-off-by: Margarit Hakobyan <[email protected]>

margarit-h changed the title ~~[WIP:] [JDBC] Fix ResultSet.getTime()~~ [DRAFT] [JDBC] Fix ResultSet.getTime() Jan 3, 2023

Yury-Fridlyand reviewed Jan 4, 2023

View reviewed changes

margarit-h closed this Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] [JDBC] Fix `ResultSet.getTime()` #192

[DRAFT] [JDBC] Fix `ResultSet.getTime()` #192

margarit-h commented Dec 16, 2022 •

edited

Loading

codecov bot commented Dec 16, 2022 •

edited

Loading

acarbonetto Dec 29, 2022

acarbonetto Dec 29, 2022

acarbonetto Dec 29, 2022

acarbonetto commented Jan 3, 2023

acarbonetto Jan 3, 2023

acarbonetto Jan 3, 2023

Yury-Fridlyand Jan 4, 2023

margarit-h commented Jan 5, 2023

[DRAFT] [JDBC] Fix ResultSet.getTime() #192

[DRAFT] [JDBC] Fix ResultSet.getTime() #192

Conversation

margarit-h commented Dec 16, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Dec 16, 2022 • edited Loading

Codecov Report

acarbonetto Dec 29, 2022

Choose a reason for hiding this comment

acarbonetto Dec 29, 2022

Choose a reason for hiding this comment

acarbonetto Dec 29, 2022

Choose a reason for hiding this comment

acarbonetto commented Jan 3, 2023

acarbonetto Jan 3, 2023

Choose a reason for hiding this comment

acarbonetto Jan 3, 2023

Choose a reason for hiding this comment

Yury-Fridlyand Jan 4, 2023

Choose a reason for hiding this comment

margarit-h commented Jan 5, 2023

[DRAFT] [JDBC] Fix `ResultSet.getTime()` #192

[DRAFT] [JDBC] Fix `ResultSet.getTime()` #192

margarit-h commented Dec 16, 2022 •

edited

Loading

codecov bot commented Dec 16, 2022 •

edited

Loading