Add: location support when connecting bigquery #10453
Conversation
Fraggle
force-pushed
the
sflory-add-region-in-credentials
branch
from
e2e0649 to
5f216ce
Compare
| @@ -68,7 +73,7 @@ export class BigQueryConnectorManager extends BaseConnectorManager<null> { | |||
| }): Promise<Result<string, ConnectorManagerError<CreateConnectorErrorCode>>> { | |||
| const credentialsRes = await getCredentials({ | |||
| credentialsId: connectionId, | |||
| isTypeGuard: isBigQueryCredentials, | |||
| isTypeGuard: isBigQueryWithLocationCredentials, | |||
There was a problem hiding this comment.
No strong opinion here, but do we really need WithLocation in the name ?
There was a problem hiding this comment.
I like to be consistent with the type name (and have 2 types one with location, the other without).
| @@ -89,6 +92,14 @@ export const fetchDatasets = async ({ | |||
| return new Ok( | |||
| removeNulls( | |||
| datasets.map((dataset) => { | |||
| // We want to filter out datasets that are not in the same location as the credentials. | |||
| // But, for example, we want to keep dataset in "us-central1" when selected location is "us" | |||
There was a problem hiding this comment.
Are we sure of this ? I thought to use a us-central1 dataset, you absolutely needed to run the query on us-central1 ? My understanding was the opposite --
If you're running the query on us-central1 then you get to also use the US multi-regional datasets.
There was a problem hiding this comment.
With the current tables based locations: If i had tables in uscentral-1 in a dataset set in us and the user selected us-central1 it would not show with an exact filter on the dataset location.
I based the location options from the tables locations, maybe it's wrong and we should define the choices on the dataset's locations only.
It's something I wanted to test asap.
| @@ -147,6 +158,12 @@ export const fetchTables = async ({ | |||
| return new Ok( | |||
| removeNulls( | |||
| tables.map((table) => { | |||
| // We want to filter out tables that are not in the same location as the credentials. | |||
| // But, for example, we want to keep tables in "us-central1" when selected location is "us" | |||
There was a problem hiding this comment.
Same, I understood the opposite.
Description
Add location selection (if needed) when connecting to BigQuery as it's not possible to do join between regions.
Store the location in the credentials to make it easy to retrieve from core.
Tests
Tested locally + added tests and updated tests.
Risk
Low as we are only touching the bigquery part.
Deploy Plan
Deploy
connectors,oauth,front