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

fix(csharp/src/Drivers/Apache): correctly handle empty response and add Client tests #2275

Merged

Conversation

birschick-bq
Copy link
Contributor

@birschick-bq birschick-bq commented Oct 23, 2024

@birschick-bq birschick-bq changed the title fix(src/Drivers/Apache): correctly handle empty response and add Client tests fix(csharp/src/Drivers/Apache): correctly handle empty response and add Client tests Oct 23, 2024
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and added tests? I have a few questions and a few nits.

csharp/test/Drivers/Apache/Spark/ClientTests.cs Outdated Show resolved Hide resolved
csharp/test/Drivers/Apache/Spark/ClientTests.cs Outdated Show resolved Hide resolved
csharp/test/Drivers/Apache/Spark/ClientTests.cs Outdated Show resolved Hide resolved
"INTERVAL 178956969 YEAR 11 MONTH as interval, " +
"ARRAY(1, 2, 3) as numbers, " +
"STRUCT('John Doe' as name, 30 as age) as person," +
"MAP('name', CAST('Jane Doe' AS STRING), 'age', CAST(29 AS INT)) as map",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this conversion happening in ADBC code or does it come that way from the server?

csharp/src/Drivers/Apache/Hive2/HiveServer2Reader.cs Outdated Show resolved Hide resolved
if ((_statement.BatchSize > 0 && fetchedRows < _statement.BatchSize) || fetchedRows == 0)
int columnCount = GetColumnCount(response);
int rowCount = GetRowCount(response, columnCount); ;
if ((_statement.BatchSize > 0 && rowCount < _statement.BatchSize) || rowCount == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't reliably get "last batch" information from TFetchResultsResp.HasMoreRows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the servers I tested, Apache Spark is setting TFetchResultsResp.HasMoreRows to false on the first and any subsequent response. I used this code as an example: https://impala.apache.org/doc/html/simba_8cc_source.html

@CurtHagenlocher CurtHagenlocher merged commit 7e4dcfa into apache:main Oct 25, 2024
6 checks passed
@birschick-bq birschick-bq deleted the dev/birschick-bq/client-tests branch November 1, 2024 18:03
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.

AdbcDataReader's Read method should return false in case of empty data source table.
2 participants