You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "davidhcoe (via GitHub)" <gi...@apache.org> on 2025/03/03 06:28:26 UTC
[PR] eat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings [arrow-adbc]
davidhcoe opened a new pull request, #2566:
URL: https://github.com/apache/arrow-adbc/pull/2566
- 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings [arrow-adbc]
Posted by "davidhcoe (via GitHub)" <gi...@apache.org>.
davidhcoe commented on code in PR #2566:
URL: https://github.com/apache/arrow-adbc/pull/2566#discussion_r1978187995
##########
csharp/src/Drivers/BigQuery/BigQueryStatement.cs:
##########
@@ -126,13 +122,13 @@ public override UpdateResult ExecuteUpdate()
QueryOptions options = ValidateOptions();
GetQueryResultsOptions getQueryResultsOptions = new GetQueryResultsOptions();
- if (this.Options?.TryGetValue(BigQueryParameters.GetQueryResultsOptionsTimeoutMinutes, out string? timeoutMinutes) == true)
+ if (this.Options?.TryGetValue(BigQueryParameters.GetQueryResultsOptionsTimeout, out string? timeoutSeconds) == true)
{
- if (int.TryParse(timeoutMinutes, out int minutes))
+ if (int.TryParse(timeoutSeconds, out int seconds))
{
- if (minutes >= 0)
+ if (seconds >= 0)
{
- getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(minutes);
+ getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(seconds);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings [arrow-adbc]
Posted by "davidhcoe (via GitHub)" <gi...@apache.org>.
davidhcoe commented on code in PR #2566:
URL: https://github.com/apache/arrow-adbc/pull/2566#discussion_r1978189010
##########
csharp/src/Drivers/BigQuery/readme.md:
##########
@@ -42,17 +42,23 @@ The following parameters can be used to configure the driver behavior. The param
https://cloud.google.com/dotnet/docs/reference/Google.Cloud.BigQuery.V2/latest/Google.Cloud.BigQuery.V2.QueryOptions#Google_Cloud_BigQuery_V2_QueryOptions_AllowLargeResults
+**adbc.bigquery.billing_project_id**<br>
+ The [Project ID](https://cloud.google.com/resource-manager/docs/creating-managing-projects) used for accessing billing BigQuery. If not specified, will default to the detected project ID.
+
**adbc.bigquery.client_id**<br>
The OAuth client ID. Required for `user` authentication.
**adbc.bigquery.client_secret**<br>
The OAuth client secret. Required for `user` authentication.
+**adbc.bigquery.client.timeout**<br>
+ Optional. Sets the timeout (in seconds) for the BigQueryClient. Similar to a ConnectionTimeout in ODBC.
Review Comment:
fixed it here and in the readme for the BigQuery test project
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings [arrow-adbc]
Posted by "davidhcoe (via GitHub)" <gi...@apache.org>.
davidhcoe commented on code in PR #2566:
URL: https://github.com/apache/arrow-adbc/pull/2566#discussion_r1978187995
##########
csharp/src/Drivers/BigQuery/BigQueryStatement.cs:
##########
@@ -126,13 +122,13 @@ public override UpdateResult ExecuteUpdate()
QueryOptions options = ValidateOptions();
GetQueryResultsOptions getQueryResultsOptions = new GetQueryResultsOptions();
- if (this.Options?.TryGetValue(BigQueryParameters.GetQueryResultsOptionsTimeoutMinutes, out string? timeoutMinutes) == true)
+ if (this.Options?.TryGetValue(BigQueryParameters.GetQueryResultsOptionsTimeout, out string? timeoutSeconds) == true)
{
- if (int.TryParse(timeoutMinutes, out int minutes))
+ if (int.TryParse(timeoutSeconds, out int seconds))
{
- if (minutes >= 0)
+ if (seconds >= 0)
{
- getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(minutes);
+ getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(seconds);
Review Comment:
I wrestled with changing this outright vs having two options. I needed something more granular for the tests, xI 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings [arrow-adbc]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #2566:
URL: https://github.com/apache/arrow-adbc/pull/2566#discussion_r1977738949
##########
csharp/src/Drivers/BigQuery/readme.md:
##########
@@ -42,17 +42,23 @@ The following parameters can be used to configure the driver behavior. The param
https://cloud.google.com/dotnet/docs/reference/Google.Cloud.BigQuery.V2/latest/Google.Cloud.BigQuery.V2.QueryOptions#Google_Cloud_BigQuery_V2_QueryOptions_AllowLargeResults
+**adbc.bigquery.billing_project_id**<br>
+ The [Project ID](https://cloud.google.com/resource-manager/docs/creating-managing-projects) used for accessing billing BigQuery. If not specified, will default to the detected project ID.
+
**adbc.bigquery.client_id**<br>
The OAuth client ID. Required for `user` authentication.
**adbc.bigquery.client_secret**<br>
The OAuth client secret. Required for `user` authentication.
+**adbc.bigquery.client.timeout**<br>
+ Optional. Sets the timeout (in seconds) for the BigQueryClient. Similar to a ConnectionTimeout in ODBC.
Review Comment:
It's a bit of a nitpick, but ODBC doesn't actually have a `ConnectionTimeout` or `CommandTimeout`. These are properties surfaced on the BCL's managed wrapper for ODBC. The ODBC standard is to set attributes on the connection and statement, respectively, with attribute identifiers of `SQL_ATTR_CONNECTION_TIMEOUT` and `SQL_ATTR_QUERY_TIMEOUT`.
As such, the analogy as written isn't necessarily helpful for someone familiar with ODBC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings [arrow-adbc]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher merged PR #2566:
URL: https://github.com/apache/arrow-adbc/pull/2566
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings [arrow-adbc]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #2566:
URL: https://github.com/apache/arrow-adbc/pull/2566#discussion_r1977723674
##########
csharp/src/Drivers/BigQuery/BigQueryStatement.cs:
##########
@@ -126,13 +122,13 @@ public override UpdateResult ExecuteUpdate()
QueryOptions options = ValidateOptions();
GetQueryResultsOptions getQueryResultsOptions = new GetQueryResultsOptions();
- if (this.Options?.TryGetValue(BigQueryParameters.GetQueryResultsOptionsTimeoutMinutes, out string? timeoutMinutes) == true)
+ if (this.Options?.TryGetValue(BigQueryParameters.GetQueryResultsOptionsTimeout, out string? timeoutSeconds) == true)
{
- if (int.TryParse(timeoutMinutes, out int minutes))
+ if (int.TryParse(timeoutSeconds, out int seconds))
{
- if (minutes >= 0)
+ if (seconds >= 0)
{
- getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(minutes);
+ getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(seconds);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(csharp/src/Drivers/BigQuery): add additional billing and timeout properties and test settings [arrow-adbc]
Posted by "CurtHagenlocher (via GitHub)" <gi...@apache.org>.
CurtHagenlocher commented on code in PR #2566:
URL: https://github.com/apache/arrow-adbc/pull/2566#discussion_r1977712590
##########
csharp/src/Drivers/BigQuery/BigQueryStatement.cs:
##########
@@ -126,13 +122,13 @@ public override UpdateResult ExecuteUpdate()
QueryOptions options = ValidateOptions();
GetQueryResultsOptions getQueryResultsOptions = new GetQueryResultsOptions();
- if (this.Options?.TryGetValue(BigQueryParameters.GetQueryResultsOptionsTimeoutMinutes, out string? timeoutMinutes) == true)
+ if (this.Options?.TryGetValue(BigQueryParameters.GetQueryResultsOptionsTimeout, out string? timeoutSeconds) == true)
{
- if (int.TryParse(timeoutMinutes, out int minutes))
+ if (int.TryParse(timeoutSeconds, out int seconds))
{
- if (minutes >= 0)
+ if (seconds >= 0)
{
- getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(minutes);
+ getQueryResultsOptions.Timeout = TimeSpan.FromMinutes(seconds);
Review Comment:
`TimeSpan.FromMinutes(seconds)` looks sus
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org