Skip to content

Conversation

@davidhcoe
Copy link
Contributor

  • adds the adbc.bigquery.client.timeout value for the BigQueryClient
  • adds the adbc.bigquery.billing_project_id value for the billing queries against
  • adds support for referencing shared values in multi-environment test configurations
  • adds tests for the new settings

@lidavidm lidavidm changed the title eat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings feat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings Mar 3, 2025
@davidhcoe davidhcoe marked this pull request as ready for review March 3, 2025 14:51
@github-actions github-actions bot added this to the ADBC Libraries 17 milestone Mar 3, 2025
if (seconds >= 0)
{
getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(minutes);
getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(seconds);
Copy link
Contributor

Choose a reason for hiding this comment

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

TimeSpan.FromMinutes(seconds) looks sus

Copy link
Contributor

Choose a reason for hiding this comment

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

Behaviorally, this would be a breaking change. Are we okay with that?

FWIW, the Snowflake driver allows setting timeouts as e.g "90s" or "1.5m" or "1m30s", which I have mixed feelings about but for which the lack of ambiguity around unit of measure is certainly nice.

Copy link
Contributor Author

@davidhcoe davidhcoe Mar 3, 2025

Choose a reason for hiding this comment

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

I wrestled with changing this outright vs having two options. I needed something more granular for the tests, I thought having two options would be confusing, but I decided to leave the two options for the BigQueryTestConfiguration because it's more likely a configuration file is set with timeoutMinutes and I just convert that. I dont think there are many, if any, consumers at this point, so I decided it was best to just make a clean break to seconds for the parameter.

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! The FromMinutes seems like something that needs attention.

@CurtHagenlocher CurtHagenlocher merged commit 15ad9d1 into apache:main Mar 3, 2025
5 of 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