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

Skip test_regexp_replace_null_pattern_fallback on Databricks [databricks] #4251

Merged

Conversation

andygrove
Copy link
Contributor

Signed-off-by: Andy Grove andygrove@nvidia.com

Closes #4243

…t Databricks

Signed-off-by: Andy Grove <andygrove@nvidia.com>
@andygrove andygrove added the build Related to CI / CD or cleanly building label Dec 1, 2021
@andygrove andygrove added this to the Nov 30 - Dec 10 milestone Dec 1, 2021
@andygrove andygrove self-assigned this Dec 1, 2021
@tgravescs
Copy link
Collaborator

tgravescs commented Dec 1, 2021

if we are disabling this and closing the issue is it tracked by another issue to turn it back on?

@tgravescs
Copy link
Collaborator

oh never mind, after looking at code looks like you are saying databricks optimizes that out, perhaps update description to say the same

@andygrove andygrove changed the title Disable test_regexp_replace_null_pattern_fallback on Databricks [databricks] Xfail test_regexp_replace_null_pattern_fallback on Databricks [databricks] Dec 1, 2021
tgravescs
tgravescs previously approved these changes Dec 1, 2021
@tgravescs
Copy link
Collaborator

build

@@ -483,11 +483,14 @@ def test_regexp_replace():
'regexp_replace(a, "a|b|c", "A")'),
conf={'spark.rapids.sql.expression.RegExpReplace': 'true'})

@pytest.mark.xfail(condition=is_databricks_runtime(),
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to xfail this or simply skip it? The point of xfailing is to track tests that we eventually expect to pass. I think skip is more appropriate for this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skipping it makes sense to me. Should I also update test_concat_ws_sql_col_sep_only_sep_specified in the same file to skip instead of xfail? (that's where I copied the pattern from).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, any test that we are never expecting to pass because it's an invalid premise for testing in a particular environment should be skipped rather than xfailed. We should reserve xfail for tests we think should pass at some point, as they essentially serve as TODO's in the test output.

@andygrove andygrove changed the title Xfail test_regexp_replace_null_pattern_fallback on Databricks [databricks] Skip test_regexp_replace_null_pattern_fallback on Databricks [databricks] Dec 1, 2021
@jlowe
Copy link
Member

jlowe commented Dec 1, 2021

build

@andygrove andygrove merged commit daa1221 into NVIDIA:branch-22.02 Dec 1, 2021
@andygrove andygrove deleted the fix-regex-failure-databricks branch December 1, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to CI / CD or cleanly building
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] test_regexp_replace_null_pattern_fallback[ALLOW_NON_GPU(ProjectExec,RegExpReplace)] failed in databricks
3 participants