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

improve array listing #9

Merged
merged 1 commit into from
Sep 8, 2023
Merged

improve array listing #9

merged 1 commit into from
Sep 8, 2023

Conversation

DimitrisStaratzis
Copy link
Contributor

@DimitrisStaratzis DimitrisStaratzis commented Aug 23, 2023 •
edited
Loading

This PR will improve the way the JDBC driver lists arrays. [sc-33884]

  • First it makes all names readable and changes the current behavior which was to display the tiledbURI.
  • Second, it now displays all public arrays as well. Unless otherwise instructed (introduced new option)
  • Lastly, fixes a bug in which the wrong namespace was used to query arrays when asking for a preview.

@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/improve_array_listing branch from 3182a5d to b3d9822 Compare August 23, 2023 14:27
@DimitrisStaratzis DimitrisStaratzis marked this pull request as draft August 23, 2023 14:27
@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/improve_array_listing branch from b3d9822 to 13a085c Compare August 23, 2023 14:29
Shelnutt2
Shelnutt2 reviewed Aug 23, 2023
View reviewed changes
src/main/java/io/tiledb/TileDBCloudConnection.java Outdated
// public arrays
if (listPublicArrays) {
try {
ArrayBrowserData resultPublic =
Copy link
Member

Choose a reason for hiding this comment

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

Could we perhaps run the listings async in the backgroup to parallelize?

Copy link
Contributor Author

@DimitrisStaratzis DimitrisStaratzis Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

you mean, load the three array categories in a different thread each and then join?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was thinking we could make the listing request concurrent in threads to reduce the latency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes absolutely, I will keep working on the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/improve_array_listing branch from 13a085c to 7daddc8 Compare September 7, 2023 15:49
@DimitrisStaratzis DimitrisStaratzis marked this pull request as ready for review September 7, 2023 15:53
@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/improve_array_listing branch 2 times, most recently from 6db4cdc to f8f3404 Compare September 7, 2023 15:56
Shelnutt2
Shelnutt2 reviewed Sep 7, 2023
View reviewed changes
src/main/java/io/tiledb/TileDBCloudTablesResultSet.java Outdated
@@ -169,15 +180,15 @@ public String getString(String columnLabel) throws SQLException {

switch (columnLabel) {
case "TABLE_NAME":
return currentArray.getTiledbUri();
return "tiledb://" + currentArray.getNamespace() + "/" + currentArray.getName();
Copy link
Member

Choose a reason for hiding this comment

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

Names aren't guaranteed to be unique, is this just for display?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just for display yes, but when the user asks for a preview the BI tools use this name to query the array. I have thought of a workaround though which I will implement first thing tomorrow.

@DimitrisStaratzis DimitrisStaratzis force-pushed the dstara/improve_array_listing branch from f8f3404 to e748189 Compare September 8, 2023 11:34
@DimitrisStaratzis
Copy link
Contributor Author

DimitrisStaratzis commented Sep 8, 2023 •
edited
Loading

To solve the name uniqueness problem, all arrays are now listed with their TileDB URI and namespace/name like this:

tiledb://dstara/orders ~ tiledb://dstara/120f0518-dd8d-467f-8c1b-23aa49929465

Whenever the JDBC driver is given such a pattern it will only use the URI with the UUID. This is achieved by using the replaceArrayNamesWithUUIDs(String input) method

Example:
Screenshot 2023-09-08 at 2 35 38 PM

Shelnutt2
Shelnutt2 approved these changes Sep 8, 2023
View reviewed changes
Copy link
Member

@Shelnutt2 Shelnutt2 left a comment

Choose a reason for hiding this comment

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

Great work!

@DimitrisStaratzis DimitrisStaratzis mentioned this pull request Sep 8, 2023
Merged
@DimitrisStaratzis DimitrisStaratzis merged commit 0473cfe into main Sep 8, 2023
@DimitrisStaratzis DimitrisStaratzis deleted the dstara/improve_array_listing branch September 8, 2023 11:58
@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #33884: Listing of the arrays when using the JDBC driver in a BI tool is incomplete..

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

@Shelnutt2 Shelnutt2 Shelnutt2 approved these changes

@ihnorton ihnorton Awaiting requested review from ihnorton

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

Successfully merging this pull request may close these issues.
2 participants
@DimitrisStaratzis @Shelnutt2