[`JDBC`] Fix precision returned for datetime columns #178

Yury-Fridlyand · 2022-11-29T04:12:59Z

Signed-off-by: Yury-Fridlyand yuryf@bitquilltech.com

Description

The proposed fix add calculation of Precision for datetime types in runtime for each dataset.
To optimize performance, analysis goes over first 1000 rows of dataset and stop earlier if precision calculated with enough precision - on 100 entries for good statistics.

Test

Before

After

Issues Resolved

opensearch-project/sql-jdbc#19

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.

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

acarbonetto · 2022-11-30T18:50:27Z

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

@@ -98,6 +99,26 @@ public ResultSetImpl(StatementImpl statement, List<? extends ColumnDescriptor> c

            List<Row> rows = getRowsFromDataRows(dataRows);

+            for (int i = 0; i < columnDescriptors.size(); i ++) {
+                if (schema.getOpenSearchType(i) == OpenSearchType.TIMESTAMP ||


if this is an enum, its better practice to use a switch

acarbonetto · 2022-11-30T18:54:02Z

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

@@ -98,6 +99,26 @@ public ResultSetImpl(StatementImpl statement, List<? extends ColumnDescriptor> c

            List<Row> rows = getRowsFromDataRows(dataRows);

+            for (int i = 0; i < columnDescriptors.size(); i ++) {


you can use array streams to filter the columns by type (DATE|TIME|DATETIME), and then get the max value row for that column.
Then you don't have to include a double for loop

Should we check to see if the Precision is already defined in the metadata beforehand?

Should we check to see if the Precision is already defined in the metadata beforehand?

No, I change the constructor.

Then you don't have to include a double for loop

I have to loop through all rows/columns anyway

Can we talk about this approach? I see what you're doing, but I have some thoughts about the pros/cons.

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

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

codecov · 2022-12-09T00:38:29Z

Codecov Report

❗ No coverage uploaded for pull request base (integ-fix-#1003@e2bf254). Click here to learn what that means.
The diff coverage is n/a.

@@                Coverage Diff                 @@
##             integ-fix-#1003     opensearch-project/sql#178   +/-   ##
==================================================
  Coverage                   ?   95.78%           
  Complexity                 ?     3465           
==================================================
  Files                      ?      357           
  Lines                      ?     9305           
  Branches                   ?      669           
==================================================
  Hits                       ?     8913           
  Misses                     ?      334           
  Partials                   ?       58

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

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

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

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

MaxKsyunz · 2022-12-09T01:48:15Z

@Yury-Fridlyand why is this the best solution? What are other options?

Seems strange that we cannot derive precision of a timestamp from OpenSearch type information.

Yury-Fridlyand · 2022-12-09T03:13:52Z

@Yury-Fridlyand why is this the best solution? What are other options?

Seems strange that we cannot derive precision of a timestamp from OpenSearch type information.

I was waiting for this question!

other options?

Return fixed values, but more realistic.

from OpenSearch type information

There is no such information, unless we change the server side to provide it.
Please, see example:

opensearchsql> select sysdate(), sysdate(3), sysdate(6), typeof(sysdate()), typeof(sysdate(3));
fetched rows / total rows = 1/1
+---------------------+-------------------------+----------------------------+---------------------+----------------------+
| sysdate()           | sysdate(3)              | sysdate(6)                 | typeof(sysdate())   | typeof(sysdate(3))   |
|---------------------+-------------------------+----------------------------+---------------------+----------------------|
| 2022-12-08 19:11:16 | 2022-12-08 19:11:16.381 | 2022-12-08 19:11:16.381169 | DATETIME            | DATETIME             |
+---------------------+-------------------------+----------------------------+---------------------+----------------------+

First 3 columns have the same type, but different precision: 19, 23 and 26.

I dislike the solution I proposed, but I have no idea how to do this better. You are welcome to share your thoughts!

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

…Time` has no FSP. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand · 2022-12-13T22:44:42Z

Discussed offline.

we can't iterate through result set;
there is no API in the SQL plugin to ask for metadata;
metadata fields should represent maximum possible values for used data types.

Fix is is implemented in 6a495cb in scope of opensearch-project#185.

First iteration.

2864fdd

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand changed the title ~~Fix precision returned for datetime columns~~ [WIP] Fix precision returned for datetime columns Nov 29, 2022

Yury-Fridlyand changed the title ~~[WIP] Fix precision returned for datetime columns~~ [WIP][JDBC] Fix precision returned for datetime columns Nov 30, 2022

acarbonetto reviewed Nov 30, 2022

View reviewed changes

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

Yury-Fridlyand added 2 commits December 8, 2022 12:28

Analyze first 100 rows.

3496550

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Fix calculating precision.

6fc0d62

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand added 3 commits December 8, 2022 16:53

Make smarter analysis.

e276f5f

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Make much smarter analysis.

67646ca

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Deactivate a test.

980fb40

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand changed the title ~~[WIP][JDBC] Fix precision returned for datetime columns~~ [JDBC] Fix precision returned for datetime columns Dec 9, 2022

Yury-Fridlyand requested review from acarbonetto, forestmvey, GabeFernandez310, GumpacG, kylepbit, margarit-h, MaxKsyunz and MitchellGale December 9, 2022 01:27

Yury-Fridlyand marked this pull request as ready for review December 9, 2022 01:27

Yury-Fridlyand added 2 commits December 9, 2022 14:24

Split analysis logic for TIMESTAMP and TIME.

4eae7b5

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Revert last commit. Precision for TIME is const, because `java.sql.…

66a5c50

…Time` has no FSP. Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Yury-Fridlyand closed this Dec 13, 2022

Yury-Fridlyand deleted the dev-fix-#1003 branch December 13, 2022 22:44

Yury-Fridlyand mentioned this pull request Dec 19, 2022

Fix default precision and maxSize for all datetime types. opensearch-project/sql-jdbc#39

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`JDBC`] Fix precision returned for datetime columns #178

[`JDBC`] Fix precision returned for datetime columns #178

Yury-Fridlyand commented Nov 29, 2022 •

edited

Loading

acarbonetto Nov 30, 2022

acarbonetto Nov 30, 2022

acarbonetto Nov 30, 2022 •

edited

Loading

Yury-Fridlyand Dec 9, 2022

Yury-Fridlyand Dec 9, 2022

kylepbit Dec 9, 2022

Yury-Fridlyand Dec 9, 2022

codecov bot commented Dec 9, 2022 •

edited

Loading

MaxKsyunz commented Dec 9, 2022

Yury-Fridlyand commented Dec 9, 2022 •

edited

Loading

Yury-Fridlyand commented Dec 13, 2022

		@@ -98,6 +99,26 @@ public ResultSetImpl(StatementImpl statement, List<? extends ColumnDescriptor> c

		List<Row> rows = getRowsFromDataRows(dataRows);

		for (int i = 0; i < columnDescriptors.size(); i ++) {

[JDBC] Fix precision returned for datetime columns #178

[JDBC] Fix precision returned for datetime columns #178

Conversation

Yury-Fridlyand commented Nov 29, 2022 • edited Loading

Description

Test

Issues Resolved

Check List

acarbonetto Nov 30, 2022

Choose a reason for hiding this comment

acarbonetto Nov 30, 2022

Choose a reason for hiding this comment

acarbonetto Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Yury-Fridlyand Dec 9, 2022

Choose a reason for hiding this comment

Yury-Fridlyand Dec 9, 2022

Choose a reason for hiding this comment

kylepbit Dec 9, 2022

Choose a reason for hiding this comment

Yury-Fridlyand Dec 9, 2022

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2022 • edited Loading

Codecov Report

MaxKsyunz commented Dec 9, 2022

Yury-Fridlyand commented Dec 9, 2022 • edited Loading

Yury-Fridlyand commented Dec 13, 2022

[`JDBC`] Fix precision returned for datetime columns #178

[`JDBC`] Fix precision returned for datetime columns #178

Yury-Fridlyand commented Nov 29, 2022 •

edited

Loading

acarbonetto Nov 30, 2022 •

edited

Loading

codecov bot commented Dec 9, 2022 •

edited

Loading

Yury-Fridlyand commented Dec 9, 2022 •

edited

Loading