Skip to content

Conversation

@toddmeng-db
Copy link
Contributor

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

A previous PR 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

@toddmeng-db toddmeng-db changed the title Remove redundant CloseOperation for GetColumnsAsync feat(csharp/src/Drivers/Databricks): Remove redundant CloseOperation for GetColumnsAsync Jul 10, 2025
EscapePatternWildcardsInName(ColumnName),
cancellationToken);
OperationHandle = resp.OperationHandle;

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 wonder if we also want to swallow CloseOperation failures. My intuition is that it's better to know when we are calling CloseOperation incorrectly (as it adds an extra network call), so we should not.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, should not swallow the exception here. but wondering do we see any test failures for this case, if not, we are missing some test coverage?

Copy link
Contributor Author

@toddmeng-db toddmeng-db Jul 10, 2025

Choose a reason for hiding this comment

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

It looks like our tests do not test for Dispose (which needs to be called explicitly). I will add some test suites

@toddmeng-db toddmeng-db force-pushed the toddmeng-db/close-operation-2 branch from 3c8d2b4 to 30b2f29 Compare July 10, 2025 23:47
@toddmeng-db toddmeng-db force-pushed the toddmeng-db/close-operation-2 branch from 30b2f29 to d04c492 Compare July 10, 2025 23:49
@toddmeng-db toddmeng-db requested a review from jadewang-db July 10, 2025 23:56
@toddmeng-db toddmeng-db marked this pull request as ready for review July 11, 2025 02:18
@github-actions github-actions bot added this to the ADBC Libraries 20 milestone Jul 11, 2025
@CurtHagenlocher CurtHagenlocher merged commit 241477e into apache:main Jul 11, 2025
9 checks passed
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