Skip to content

Conversation

@toddmeng-db
Copy link
Contributor

@toddmeng-db toddmeng-db commented Jul 3, 2025

The driver was breaking with DBR 11.3.

This PR introduced logic which throws errors via HandleThriftResponse. Previously, we had been silently ignoring these CloseOperation failures; these errors occurred because the operations can be closed when returning via DirectResults, and we weren't properly checking for DirectResults.CloseOperation status for Metadata queries. With this change, we avoid making redundant CloseOperation requests upon dispose.

  1. Metadata Request to server
  2. Metadata Response with DirectResults and CloseOperation
  3. Statement.Dispose
  4. DirectResults includes CloseOperation
  5. Skip redundant CloseOperation

Testing

Added a test that only passes with the changes in this PR in DBR 11.3

private async Task<QueryResult> GetQueryResult(TSparkDirectResults? directResults, CancellationToken cancellationToken)
{
// Set _directResults so that dispose logic can check if operation was already closed
_directResults = directResults;
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we don't have test here, can we add one?

@toddmeng-db toddmeng-db force-pushed the toddmeng-db/closeop-error branch from d32db4a to b57282b Compare July 3, 2025 18:12
@toddmeng-db toddmeng-db marked this pull request as ready for review July 3, 2025 18:47
@github-actions github-actions bot added this to the ADBC Libraries 20 milestone Jul 3, 2025
@toddmeng-db toddmeng-db requested a review from jadewang-db July 3, 2025 18:57
@CurtHagenlocher CurtHagenlocher merged commit e93d181 into apache:main Jul 3, 2025
19 checks passed
CurtHagenlocher pushed a commit that referenced this pull request Jul 11, 2025
…for GetColumnsAsync (#3132)

[A previous PR](#3093) fixed
redundant CloseOperations that were throwing errors and causing failure
for dbr 11.3 and below.

GetColumnsAsync uses a seperate response handling path, which does not
call GetQueryResults (where the previous fix was implemented), so this
operation still fails. This PR adds a fix
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