Skip to content

fix(csharp/src/Drivers/BigQuery): Fix the bug about large result and timeout #2810

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.

Sign up for GitHub

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

Merged
CurtHagenlocher merged 1 commit into from
May 12, 2025
Merged

fix(csharp/src/Drivers/BigQuery): Fix the bug about large result and timeout #2810

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 4 additions & 23 deletions csharp/src/Drivers/BigQuery/BigQueryStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,27 +93,8 @@ private async Task<QueryResult> ExecuteQueryInternalAsync()
getQueryResultsOptions.Timeout = TimeSpan.FromSeconds(seconds);
}

Func<Task<BigQueryJob>> checkJobStatus = async () =>
{
while (true)
{
var jobWithStatus = await Client.GetJobAsync(jobReference);

if (jobWithStatus.State == JobState.Done)
{
if (jobWithStatus.Status.ErrorResult != null)
{
// TODO: log
Debug.WriteLine($"Error: {jobWithStatus.Status.ErrorResult.Message}");
}

return jobWithStatus;
}
}
};

await ExecuteWithRetriesAsync<BigQueryJob>(checkJobStatus);

// We can't checkJobStatus, Otherwise, the timeout in QueryResultsOptions is meaningless.

@CurtHagenlocher CurtHagenlocher May 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would removing the polling loop not break the case where there's a longer-running job?. To the extent that there's a problem with the query timeout, should that not instead be reflected in the "while true" logic that doesn't currently take a timeout into account?

@davidhcoe, waiting for your feedback on this.

@qifanzhang-ms qifanzhang-ms May 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The change in timeout behavior is caused by this commit: feat(csharp/src/Drivers/BigQuery): Add support for AAD/Entra authenti… · apache/arrow-adbc@5aa9f1e.
Let me explain the changes before and after this commit based on my understanding. In the previous timeout feature, it was completely specified by the timeout parameter in QueryResultOptions (default is five minutes, if the job is not completed after 5 minutes, it will prompt timeout), but customers can freely set it (such as setting timeout after 1 minute, timeout after 15 minutes, etc.). But after the modification, it became a constant wait and completely uncontrollable.
I don't think it's necessary to add relevant logic in while true, because the official QueryResultOptions already has a timeout parameter to implement this function, and we have been using it before without encountering any problems.
Of course, this is my viewpoint, and I look forward to your opinions.

@davidhcoe davidhcoe May 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I will take a look.

@davidhcoe davidhcoe May 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good to me @CurtHagenlocher . I validated the long running query will still succeed without that block in place, and it will time out when it is supposed to time out.

// When encountering a long-running job, it should be controlled by the timeout in the Google SDK instead of blocking in a while loop.
Func<Task<BigQueryResults>> getJobResults = async () =>
{
// if the authentication token was reset, then we need a new job with the latest token
Expand Down Expand Up @@ -224,7 +205,6 @@ public override UpdateResult ExecuteUpdate()

private async Task<UpdateResult> ExecuteUpdateInternalAsync()
{
QueryOptions options = ValidateOptions();
GetQueryResultsOptions getQueryResultsOptions = new GetQueryResultsOptions();

if (Options?.TryGetValue(BigQueryParameters.GetQueryResultsOptionsTimeout, out string? timeoutSeconds) == true &&
Expand All @@ -234,7 +214,8 @@ private async Task<UpdateResult> ExecuteUpdateInternalAsync()
getQueryResultsOptions.Timeout = TimeSpan.FromSeconds(seconds);
}

Func<Task<BigQueryResults?>> func = () => Client.ExecuteQueryAsync(SqlQuery, null, options, getQueryResultsOptions);
// Cannot set destination table in jobs with DDL statements, otherwise an error will be prompted
Func<Task<BigQueryResults?>> func = () => Client.ExecuteQueryAsync(SqlQuery, null, null, getQueryResultsOptions);
BigQueryResults? result = await ExecuteWithRetriesAsync<BigQueryResults?>(func);
long updatedRows = result?.NumDmlAffectedRows.HasValue == true ? result.NumDmlAffectedRows.Value : -1L;

Expand Down