-
Notifications
You must be signed in to change notification settings - Fork 4
[PF-2195] Switch default BQ resolve delimiter and add a FULL_PATH_SQL option. #351
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
base: main
Are you sure you want to change the base?
Conversation
melissachang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure which should be default. It could be that a user rarely runs bq, and often runs SQL queries (eg in a notebook) against data collection BQ resources. If you want, I'm happy to shoot an email to solutions. The email could kill 2 birds with 1 stone: ask for their preference, and possibly notify them of breaking change.
If we keep bq as default, I can update UI to match. (In the future, this logic should be in FrontendServer.)
| /** This enum specifies the possible ways to resolve a BigQuery resource. */ | ||
| public enum BqResolvedOptions { | ||
| FULL_PATH, // For data table: [project id].[dataset id].[data table id if applicable] | ||
| FULL_PATH, // For data table: [project id]:[dataset id].[data table id if applicable] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest FULL_PATH -> FULL_PATH_BQ and some comment changes
// project:dataset.table, used for bq CLI
FULL_PATH_BQ,
// project.dataset.table, used for SQL
FULL_PATH_SQL,
(I left out if applicable for brevity..)
|
|
||
| /** Gets cloud identifier for Dataset in full-path: [project id].[dataset id] */ | ||
| /** Gets cloud identifier for Dataset in full-path: [project id]:[dataset id] */ | ||
| public static String getDatasetFullPath(String projectId, String datasetId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getDatasetFullPathBq ?
👍 I emailed the solutions team, and will come back to this PR once we have a clearer signal on which default to prefer. |
See associated ticket for a full description of the issue.
tl;dr it will be more consistent for our CLI to resolve BQ datasets and tables using the format:
[project_id]:[dataset_id].[opt_table_id]This is the format expected by the
bqcommand-line, and should be Terra's default output.At the same time, for anyone who wants to include a Terra reference within BigQuery SQL, they should use the format we used to output, e.g.
[project_id].[dataset_id].[opt_table_id]This PR switches the default behavior and adds a new
--bq-pathoption to specify the SQL-compatible output format.We should notify our Solutions team before this change goes live, as it will change the default BQ resource resolution.
(Note that this PR is intentionally making a breaking change in a core command. This should be frowned upon as a general matter of course! However, I am high confidence that the existing behavior is incorrect to the point of being actively misleading, and as a result the existing resolve command is not likely relied-upon by our small existing user base. The benefit to adjusting sooner than later outweighs the cost of breaking backwards compatibility.)