bigquery live connector export fix#9400
Conversation
| func BigQueryEscapeAlias(alias string) string { | ||
| return BigQueryEscapeIdentifier(BigQueryAliasName(alias)) | ||
| } | ||
|
|
||
| // BigQueryAliasName returns the column name BigQuery produces when the given string is used as a SELECT alias. | ||
| // BigQuery flexible column names disallow certain characters; runs of disallowed characters are collapsed to "__" | ||
| // and any trailing underscores are trimmed. | ||
| func BigQueryAliasName(alias string) string { | ||
| return strings.TrimRight(bqRestrictedRunRegex.ReplaceAllString(alias, "__"), "_") | ||
| } |
There was a problem hiding this comment.
I don't understand how this is safe – isn't EscapeAlias sometimes called for non-display-name columns? If so, then won't this cause it to fail silently instead of error?
I had expected a dialect.SanitizeDisplayName function or something like that, which would explicitly be called in the places where UseDisplayNames is checked. To ensure that we never rewrite columns unless they are display names (because with display names we know/assume that people are not expecting perfectly correct names)
| // bqRestrictedRunRegex matches characters that are NOT supported in | ||
| // BigQuery flexible column names. See: | ||
| // https://cloud.google.com/bigquery/docs/schemas#flexible-column-names | ||
| var bqRestrictedRunRegex = regexp.MustCompile(`[!"$()*,./;?@\[\\\]^` + "`" + `{}~\s]+`) |
There was a problem hiding this comment.
The term "restricted run regex" doesn't feel very informative. What about something like restrictedAliasCharactersRegex? (Btw, when code is inside a package named "bigquery", you don't need to prefix variables with "bq" – it's implied.)
begelundmuller
left a comment
begelundmuller
left a comment
There was a problem hiding this comment.
Very minor nit, up to you if you want to handle or leave as it is.
* bigquery live connector export fix * fix for all display names * self revie * review comments * self review * review comments
2 participants
closes https://linear.app/rilldata/issue/PLAT-481/fix-exports-for-metrics-on-bigquery
Checklist: