Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adding Array Support including Unit Tests #81

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Adding Array Support including Unit Tests #81

wants to merge 2 commits into from

Conversation

akuzin1
Copy link
Contributor

@akuzin1 akuzin1 commented May 23, 2023 •
edited
Loading

Description

Even though there is no dedicated array field type in OpenSearch, one can still pass an array of values into any field. However, currently there is no way to take advantage of that feature. Therefore, I added Array support to the driver, so that the user can call getArray(<column-index>) to retrieve the array object stored as a field from the returned ResultSet.

Issues Resolved

[List any issues this PR will resolve]

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.

Yury-Fridlyand
Yury-Fridlyand reviewed Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few small comments. I'm going to test this manually to verify.
For documentation purposes can you add to PR description screenshots how it looks with and without this fix? Please add you sample data and mapping to let others verify the test.
Thanks

src/main/java/org/opensearch/jdbc/ArrayImpl.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/jdbc/ArrayImpl.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/jdbc/ResultSetImpl.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/jdbc/ResultSetImpl.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/jdbc/ResultSetImpl.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/jdbc/ResultSetImpl.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/jdbc/ResultSetImpl.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/jdbc/types/ArrayType.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/jdbc/types/OpenSearchType.java
@@ -61,15 +63,17 @@ public enum OpenSearchType {
STRING(JDBCType.VARCHAR, String.class, Integer.MAX_VALUE, 0, false),
IP(JDBCType.VARCHAR, String.class, 15, 0, false),
NESTED(JDBCType.STRUCT, null, 0, 0, false),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe nested should be mapped to array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why? I may not be following.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested is mapped to ARRAY in sql plugin, curious why it is different here?
OpenSearchDataType.java#L39

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sql plugin behavior always returns only the first element, doesn't actually treat it as an Array...
Screenshot 2023-06-02 at 2 07 35 PM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bug of workbench?

@akuzin1
Added Array Support to driver as well as Unit Tests for new functiona…
a6780b5 
…lity

Signed-off-by: Andrey Kuzin <akuzin@amazon.com>
@akuzin1
Copy link
Contributor Author

akuzin1 commented Jun 2, 2023

The sample data I used to populate OpenSearch Index:

POST sample_array_data/_doc
{
  "int_array": [1, 2, 3],
  "string_array": ["abc", "cde", "fgh"],
  "double_array": [1.25, 5.99, 12.32],
  "boolean_array": [true, false, true]
}

POST sample_array_data/_doc
{
  "int_array": [4, 5, 6],
  "string_array": ["qrs", "tuv", "xyz"],
  "double_array": [7.23, 10.50, 99.99],
  "boolean_array": [false, true, false]
}

Before:
Screenshot 2023-06-02 at 1 05 14 PM

java.lang.RuntimeException: java.sql.SQLFeatureNotSupportedException: Array is not supported
	at com.amazonaws.athena.connector.lambda.data.S3BlockSpiller.writeRows(S3BlockSpiller.java:196)
	at com.amazonaws.athena.connectors.jdbc.manager.JdbcRecordHandler.readWithConstraint(JdbcRecordHandler.java:185)
	at com.amazonaws.athena.connector.lambda.handlers.RecordHandler.doReadRecords(RecordHandler.java:197)
	at com.amazonaws.athena.connector.lambda.handlers.RecordHandler.doHandleRequest(RecordHandler.java:163)
	at com.amazonaws.athena.connector.lambda.handlers.CompositeHandler.handleRequest(CompositeHandler.java:147)
	at com.amazonaws.athena.connector.lambda.handlers.CompositeHandler.handleRequest(CompositeHandler.java:112)
Caused by: java.sql.SQLFeatureNotSupportedException: Array is not supported
	at org.opensearch.jdbc.ResultSetImpl.getArray(ResultSetImpl.java:1053)
	at com.amazonaws.athena.connectors.jdbc.manager.JdbcRecordHandler.lambda$makeFactory$1(JdbcRecordHandler.java:206)
	at com.amazonaws.athena.connector.lambda.data.writers.GeneratedRowWriter.writeRow(GeneratedRowWriter.java:119)
	at com.amazonaws.athena.connectors.jdbc.manager.JdbcRecordHandler.lambda$readWithConstraint$0(JdbcRecordHandler.java:185)
	at com.amazonaws.athena.connector.lambda.data.S3BlockSpiller.writeRows(S3BlockSpiller.java:193)
	... 5 more

After:
Screenshot 2023-06-02 at 12 32 31 PM

Yury-Fridlyand
Yury-Fridlyand reviewed Jun 2, 2023
View reviewed changes
src/main/java/org/opensearch/jdbc/types/ArrayType.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/jdbc/types/OpenSearchType.java
@@ -61,15 +63,17 @@ public enum OpenSearchType {
STRING(JDBCType.VARCHAR, String.class, Integer.MAX_VALUE, 0, false),
IP(JDBCType.VARCHAR, String.class, 15, 0, false),
NESTED(JDBCType.STRUCT, null, 0, 0, false),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bug of workbench?

@akuzin1 @Yury-Fridlyand
Update src/main/java/org/opensearch/jdbc/types/ArrayType.java
9ff2b30 
Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: akuzin1 <111004764+akuzin1@users.noreply.github.com>
@Yury-Fridlyand Yury-Fridlyand marked this pull request as draft June 7, 2023 20:25
@Yury-Fridlyand
Copy link
Collaborator

I suspended this PR, because it is blocked by opensearch-project/sql#1300. We need to implement this feature in SQL plugin prior to make any changes in JDBC.

Yury-Fridlyand added a commit to Bit-Quill/sql-jdbc that referenced this pull request Jun 8, 2023
@Yury-Fridlyand
Remove opensearch-project#81.
ae22a46 
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand left review comments

@forestmvey forestmvey forestmvey left review comments

@pjfitzgibbons pjfitzgibbons Awaiting requested review from pjfitzgibbons

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm kavithacm is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho is a code owner

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@rupal-bq rupal-bq Awaiting requested review from rupal-bq

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@vamsi-amazon vamsi-amazon Awaiting requested review from vamsi-amazon vamsi-amazon is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az seankao-az is a code owner

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz MaxKsyunz is a code owner

@anirudha anirudha Awaiting requested review from anirudha anirudha will be requested when the pull request is marked ready for review anirudha is a code owner

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto will be requested when the pull request is marked ready for review acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG GumpacG will be requested when the pull request is marked ready for review GumpacG is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akuzin1 @Yury-Fridlyand @forestmvey