Skip to content

Conversation

@jackyhu-db
Copy link
Contributor

Motivation

When the client calls HiveServer2Statement to execute metadata query (e.g. GetCatalogsAsync, GetSchemasAsync), it won't set _directResults, which leads to a unnecessary thrift call (CloseOperation) in its HiveServer2Statement.Dispose, this is because it will make a blocking call CloseOperation if _directResults=null (see code here), it introduces 100ms-200ms latency for every metadata call/query

Change

Set HiveServer2Statement._directResults from resp.DirectResult in every metadata API in HiveServer2Statement, thus thrift CloseOperation won't be called in Dispose if metadata query/option returns a successful DirectResult.

Testing

  • All the E2E testing on Databricks driver
  • PGPerf.exe and saw 200-300ms performance improvement for every query scenario with Databricks

@github-actions github-actions bot added this to the ADBC Libraries 20 milestone Jul 22, 2025
@CurtHagenlocher CurtHagenlocher changed the title fix(csharp/src/Drivers/Hive2): Remove unnecessary CloseOperation in Statement.Dispose when query is metadata query fix(csharp/src/Drivers/Apache/Hive2): Remove unnecessary CloseOperation in Statement.Dispose when query is metadata query Jul 22, 2025
@jackyhu-db jackyhu-db changed the title fix(csharp/src/Drivers/Apache/Hive2): Remove unnecessary CloseOperation in Statement.Dispose when query is metadata query fix(csharp/src/Drivers/Apache/Hive2): Remove unnecessary CloseOperation in Statement.Dispose when query is metadata query Jul 22, 2025
@CurtHagenlocher
Copy link
Contributor

This is definitely not needed for GetCrossReferenceAsForeignTableAsync? Should there be a comment to that effect?

@jackyhu-db
Copy link
Contributor Author

This is definitely not needed for GetCrossReferenceAsForeignTableAsync? Should there be a comment to that effect?

If we do not set this, CloseOperation will be called if the client calls GetCrossReferenceAsForeignTableAsync

@CurtHagenlocher
Copy link
Contributor

This is definitely not needed for GetCrossReferenceAsForeignTableAsync? Should there be a comment to that effect?

If we do not set this, CloseOperation will be called if the client calls GetCrossReferenceAsForeignTableAsync

Sorry, I didn't word that very well. Every method that calls GetQueryResult was changed except for GetCrossReferenceAsForeignTableAsync so I just wanted to confirm that this is correct.

@jackyhu-db
Copy link
Contributor Author

This is definitely not needed for GetCrossReferenceAsForeignTableAsync? Should there be a comment to that effect?

If we do not set this, CloseOperation will be called if the client calls GetCrossReferenceAsForeignTableAsync

Sorry, I didn't word that very well. Every method that calls GetQueryResult was changed except for GetCrossReferenceAsForeignTableAsync so I just wanted to confirm that this is correct.

Thanks for catching this. It should be added as well, uploaded a fix.

@CurtHagenlocher CurtHagenlocher merged commit cd2f264 into apache:main Jul 23, 2025
6 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