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.

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

feat(java/driver/flight-sql): implement getObjects #605

Closed
wants to merge 4 commits into from

Conversation

tokoko
Copy link
Contributor

@tokoko tokoko commented Apr 22, 2023

implements getObjects in flight-sql driver.

There are some limitations that I hope is okay to postpone, namely catalogs and schemas are being left out if they don't contain any tables and don't show up in FlightSql's getTables output. This shouldn't be too hard to rectify, but is currently tricky to test with just sqlite validation, because you can't have more than one database in sqlite. I'll add necessary tests and a fix later on as part of Dremio validation suite.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks! I left some general comments.

Comment on lines 96 to 102
private void writeVarChar(VarCharWriter writer, String value) {
byte[] bytes = value.getBytes(StandardCharsets.UTF_8);
try (ArrowBuf tempBuf = allocator.buffer(bytes.length)) {
tempBuf.setBytes(0, bytes, 0, bytes.length);
writer.writeVarChar(0, bytes.length, tempBuf);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we don't just upstream this (at some point). This bit of code gets written so much for no gain...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take a stab at it later. writeVarChar(Text text) method is actually part of VarCharWriterImpl already, but it's not exposed through VarCharWriter interface for some reason. Most of the codebase around writers is code generated, so this might actually be an oversight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jduo looks like you already fixed this with 38614, but adbc still depends on version 14. Can we bump arrow dependency version as part of this PR or should that be separate?

byte[] lastDbSchemaAdded = null;
int catalogIndex = 0;

for (FlightEndpoint endpoint : info.getEndpoints()) {
Copy link
Member

Choose a reason for hiding this comment

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

What other drivers have done is factor out the reader logic from the statement so that it doesn't need to be replicated here. Though maybe it's simple enough that it isn't a big deal.

FlightStream stream = client.getStream(endpoint.getTicket());
while (stream.next()) {
try (VectorSchemaRoot res = stream.getRoot()) {
VarCharVector catalogVector = (VarCharVector) res.getVector(0);
Copy link
Member

Choose a reason for hiding this comment

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

Validate the schema somewhere before casting?

@jduo
Copy link
Member

jduo commented Jan 22, 2024

@tokoko , this issue hasn't been updated since May. May I help finish getting this one merged?

@github-actions github-actions bot added this to the ADBC Libraries 0.10.0 milestone Jan 22, 2024
@tokoko
Copy link
Contributor Author

tokoko commented Jan 23, 2024

@jduo sure, go for it. just resolved conflicts with main and some small comments. thanks

@jduo
Copy link
Member

jduo commented Feb 5, 2024

@tokoko I'm picking this up now. Work is on #1517 .

@lidavidm
Copy link
Member

Superseded by #1517

@lidavidm lidavidm closed this Feb 29, 2024
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.

3 participants