Skip to content

Conversation

@chris-allan
Copy link
Contributor

Follow on from #196 and support some more string encodings. Also when a particular encoding is chosen (ISO-8859-1 in particular), use it rather than always encoding in UTF-8. I am assuming this was the intent for the TILEDB_STRING_ASCII type but let me know whether pure US-ASCII was actually intended. This should make TileDB-Java compatible with all the string encodings that are currently possible via TileDB-Py.

/cc @kkoz, @erindiel

@gsvic gsvic self-requested a review September 30, 2020 12:44
Copy link
Contributor

@gsvic gsvic left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @chris-allan! The PR looks good, the only concern is that we should preferably use the ISO_8859_1 encoding in both TILEDB_STRING_ASCII and TILEDB_CHAR as both are intended to be ASCII and use the UTF_8 encoding only in TILEDB_UTF8 data type. + @Shelnutt2

@chris-allan
Copy link
Contributor Author

No problem. Just let me know what you decide and I can make that change. Since I think everything is UTF-8 encoded on the Python side there's really not a precedent set for the ASCII encodings there.

@gsvic
Copy link
Contributor

gsvic commented Sep 30, 2020

No problem. Just let me know what you decide and I can make that change. Since I think everything is UTF-8 encoded on the Python side there's really not a precedent set for the ASCII encodings there.

Hi @chris-allan. If you could just make a change for handling TILEDB_CHAR and TILEDB_STRING_ASCII the same way (ISO_8859_1) it would be great. This is the only change needed.

@gsvic gsvic requested a review from Shelnutt2 September 30, 2020 13:51
Copy link
Contributor

@gsvic gsvic left a comment

Choose a reason for hiding this comment

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

LGTM!

@chris-allan
Copy link
Contributor Author

@Shelnutt2 / @gsvic: Anything else you would like done here?

@gsvic gsvic merged commit 0883797 into TileDB-Inc:master Oct 13, 2020
@gsvic
Copy link
Contributor

gsvic commented Oct 13, 2020

@Shelnutt2 / @gsvic: Anything else you would like done here?

Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants