[`JDBC`] Fix precision returned for datetime columns by Yury-Fridlyand · Pull Request #178 · Bit-Quill/opensearch-project-sql

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

            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


            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 GabeFernandez310, GumpacG, MaxKsyunz, MitchellGale, acarbonetto, forestmvey, kylepbit and margarit-h 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 wants to merge 8 commits intointeg-fix-#1003from
dev-fix-#1003

Yury-Fridlyand commented Nov 29, 2022 •

edited

Loading

Uh oh!

acarbonetto Nov 30, 2022

Uh oh!

acarbonetto Nov 30, 2022

Uh oh!

acarbonetto Nov 30, 2022 •

edited

Loading

Uh oh!

Yury-Fridlyand Dec 9, 2022

Uh oh!

Yury-Fridlyand Dec 9, 2022

Uh oh!

kylepbit Dec 9, 2022

Uh oh!

Yury-Fridlyand Dec 9, 2022

Uh oh!

Uh oh!

codecov bot commented Dec 9, 2022 •

edited

Loading

Uh oh!

MaxKsyunz commented Dec 9, 2022

Uh oh!

Yury-Fridlyand commented Dec 9, 2022 •

edited

Loading

Uh oh!

Yury-Fridlyand commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		List<Row> rows = getRowsFromDataRows(dataRows);

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

Conversation

Yury-Fridlyand commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test

Issues Resolved

Check List

Uh oh!

acarbonetto Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

acarbonetto Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

acarbonetto Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yury-Fridlyand Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

Yury-Fridlyand Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

kylepbit Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

Yury-Fridlyand Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MaxKsyunz commented Dec 9, 2022

Uh oh!

Yury-Fridlyand commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yury-Fridlyand commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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