Add support for regex flags for BigQuery #253
Conversation
clausherther
added
enhancement
|
Thanks! Will take a closer look tomorrow or Thursday. |
|
Test failed for Snowflake I should've read the Snowflake docs more carefully, sorry. Snowflake, since it's using POSIX ERE, does not support inline flags. |
| @@ -18,7 +18,14 @@ models: | |||
| regex: "[A-Z]" | |||
| flags: i | |||
| config: | |||
| enabled: "{{ target.type in ['postgres', 'snowflake', 'redshift' ] }}" | |||
| enabled: "{{ target.type in ['postgres', 'snowflake', 'redshift', 'bigquery' ] }}" | |||
There was a problem hiding this comment.
This covers all of our supported platforms, I'm wondering if we should just remove the enabled config for this test then?
There was a problem hiding this comment.
I've now removed all occurrences of that enabled config
| @@ -59,7 +66,7 @@ models: | |||
| regex_list: ["[A-G]", "[H-Z]"] | |||
| flags: i | |||
| config: | |||
| enabled: "{{ target.type in ['postgres', 'snowflake', 'redshift' ] }}" | |||
| enabled: "{{ target.type in ['postgres', 'snowflake', 'redshift', 'bigquery' ] }}" | |||
There was a problem hiding this comment.
same comment here re: removed config
| {% if not is_match %} | ||
| {# Using raise_compiler_error causes disabled tests with invalid flags to fail compilation #} | ||
| {{ exceptions.warn( | ||
| "flags " ~ flags ~ " isn't a valid re2 flag pattern" |
There was a problem hiding this comment.
if a user passes in an unsupported flag, like c on BigQuery, this will generate a compiler warning, but the test will ultimately fail (e.g. "Cannot parse regular expression: invalid perl operator: (?c"). Is that the intent?
There was a problem hiding this comment.
The issue if we raise a compiler error is that the tests don't compile anymore.
As you can see in the pipeline test, even though the test for the "c" flag is not enabled for BigQuery (since it isn't a valid re2 flag), we have a warning in the output.
02:04:16 flags c isn't a valid re2 flag pattern
02:04:16 flags c isn't a valid re2 flag pattern
02:04:16 flags c isn't a valid re2 flag pattern
Here's the output with raise_compiler_error instead of warn:
Encountered an error:
Compilation Error in test dbt_expectations_expect_column_values_to_match_regex_data_text_email_address__c___A_Z_ (models/schema_tests/schema.yml)
flags c isn't a valid re2 flag pattern
Having a warning is nice since the test will compile properly but it can be forced to error with the --warn-error flag.
i.e.:
running dbt --warn-error build will fail build for invalid flags in test (even if the invalid-flag tests are disabled).
running dbt build will build for invalid flags in test (but display a warning message and ultimately fail)
However, I get that this might be considered a regression since it changes the default behavior to be more lax.
This gives us 2 options:
- Remove the "c" flag test
- Add support for the "c" flag in BigQuery by translating it to "-i". This improves adapter flexibility but might be unintuitive for the user since "c" is not supposed be a valid re2 flag.
There was a problem hiding this comment.
Sorry for the delayed response. I think it's to leave as is, including the c flag tests with the defined targets. If we find that this creates usability issues for folks, we can address in a later PR (and maybe implement option 2). Thanks for the thorough explanation!
What does this PR do?
Add support for regex flags for BigQuery (#252)
Change description
Inline flags in non-capturing block
Type of change
Where has this been tested?