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): choose the first project ID if not specified #2541

Merged

Conversation

davidhcoe
Copy link
Contributor

  • The call to BigQueryClient.CreateQueryJob fails if a project ID is not present (even though the *detect-project-id* value is passed). This change locates the first project ID in the list of project IDs and uses it if no project ID is specified.
  • Adds support for multiple BigQuery test environments. Includes some refactoring for of Apache.Arrow.Adbc.Tests.Drivers.Interop.FlightSql to move common functionality to a shared library.

@github-actions github-actions bot added this to the ADBC Libraries 17 milestone Feb 20, 2025
@lidavidm lidavidm changed the title eat(csharp/src/Drivers/BigQuery): choose the first project ID if not specified feat(csharp/src/Drivers/BigQuery): choose the first project ID if not specified Feb 20, 2025
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! I left feedback.

public List<string> TestEnvironmentNames { get; set; } = new List<string>();

/// <summary>
/// The active test environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment seems to be at odds with both the property definition and the sample file.

What would it mean for the keys of this dictionary to be different than the list of names in TestEnvironmentNames?

}

if (environments.Count == 0)
throw new InvalidOperationException("Could not find a configured Flight SQL environment");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this error message more specific than it should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@lidavidm lidavidm removed this from the ADBC Libraries 17 milestone Mar 3, 2025
@CurtHagenlocher
Copy link
Contributor

The C# test failure looks unrelated.

@CurtHagenlocher CurtHagenlocher merged commit 80094ce into apache:main Mar 3, 2025
5 of 6 checks passed
@lidavidm lidavidm added this to the ADBC Libraries 18 milestone Mar 3, 2025
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