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

feat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings #2566

Merged

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
{
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