skip failing tests default to false by comphead · Pull Request #3890 · apache/datafusion

comphead · 2022-10-19T00:24:27Z

Which issue does this PR close?

Closes#3695.

Rationale for this change

skip_failing_rules default to true in tests so that regressions are caught much sooner

What changes are included in this PR?

Are there any user-facing changes?

comphead · 2022-10-19T00:25:08Z

@andygrove please check this PR. I have commented out 1 test as it fails, will create another PR to fix the test

# Conflicts: # datafusion/core/src/config.rs

alamb

Thank you @AssHero

My only concern about this approach is that now we are not testing the same thing that users are.

What would we feel about changing the default value of OPT_OPTIMIZER_SKIP_FAILED_RULES to always be true and downstream projects could turn it to true if they wanted.

What do you think @avantgardnerio ?

alamb · 2022-10-19T18:50:49Z

 false,
 ),
+ #[cfg(test)]
+ ConfigDefinition::new_bool(


Suggested change
ConfigDefinition::new_bool(
// skip_failing_rules default to true in tests so that regressions are caught much sooner
ConfigDefinition::new_bool(

alamb · 2022-10-19T18:53:31Z

+ "When set to true, the logical plan optimizer will produce warning \
+ messages if any optimization rules produce errors and then proceed to the next \
+ rule. When set to false, any rules that produce errors will cause the query to fail.",


Suggested change
"When set to true, the logical plan optimizer will produce warning \
messages if any optimization rules produce errors and then proceed to the next \
rule.When set to false, any rules that produce errors will cause the query to fail.",
"OVERRIDDEN FOR TEST",

I think this could be very confusing to people (that tests and defaults are different) so I suggest some more comments to make it clearer

alamb · 2022-10-19T18:54:57Z

 Ok(())
 }

+ #[ignore]


We should have a ticket to track this failure. How does it fail with the change in default?

did not mean to approve -- I think we should default to true always

alamb · 2022-10-19T18:57:21Z

 to reduce the number of rows decoded.",
 false,
 ),
+ #[cfg(test)]


I think this #[cfg(test)] will effectively only apply to tests in the datafusion/core crate -- this option will be false for other tests (like sql_integration for example)

comphead · 2022-10-19T19:37:45Z

Thanks @alamb for the feedback.
I'm not an @AssHero though :)
I like the idea to default to FALSE always, so we dont skip any unexpected behaviuor. For all failing tests, lets ignore them for now to avoid build fail, and I can create separate tickets for each failing test.

andygrove · 2022-10-19T20:52:34Z

What would we feel about changing the default value of OPT_OPTIMIZER_SKIP_FAILED_RULES to always be true

I think that would be problematic given the current maturity of the optimization rules. It would result in many queries failing rather than just skipping some optimizations.

alamb · 2022-10-19T20:54:01Z

I'm not an @AssHero though :)

🤦 I apologize to both of you

andygrove

I think tests should be explicit about the configs they are testing. I don't think we should have different default values for configs between test and production code.

comphead · 2022-10-19T21:14:51Z

I'm wondering what is the point of test that works with enabled skip_failing_rules, but will fail otherwise.....
Shouldn't be they just ignored and fixed later, as now it looks misleading a bit

comphead · 2022-10-20T15:09:20Z

I think tests should be explicit about the configs they are testing. I don't think we should have different default values for configs between test and production code.

Sorry @andygrove, I lost a bit. So you also support disabling failing rules check, ignore tests, and create a new PR to fix them?

andygrove · 2022-10-21T14:45:43Z

Currently, if any rule fails with an Err then the optimizer just skips that rule, so the user does not see an error and their query works (but maybe not fully optimized). I think this is the correct behavior for now because we know that there are bugs in some of the rules.

I would like to have regression tests to that we don't introduce any new bugs that cause rules to fail. One approach would be to set datafusion.optimizer.skip_failed_rules=false in all the tests where that does not cause a test failure, and make sure we have issues filed for tests that would fail with that setting and add a comment to those tests, linking to the issue.

Long term I would like the setting to default to false, but we need to fix the bugs first.

comphead · 2022-10-22T00:51:32Z

@andygrove got your point thanks. Please check renewed PR

So idea is by default tests run with enabled skip_optimization_errors, to make them fail instead of warning, one can run tests specyfing the conf explicitly as cargo test --features "optimizer_failed_rules"

comphead · 2022-10-22T00:51:44Z

@andygrove got your point thanks. Please check renewed PR

So idea is by default tests run with enabled skip_optimization_errors, to make them fail instead of warning, one can run tests specyfing the conf explicitly as cargo test --features "optimizer_failed_rules"

comphead · 2022-10-25T15:58:18Z

@andygrove please check until the code rots

alamb · 2022-10-27T14:27:54Z

Marking as a draft as it is waiting on review from @andygrove

andygrove · 2022-10-27T16:21:19Z

@andygrove got your point thanks. Please check renewed PR
So idea is by default tests run with enabled skip_optimization_errors, to make them fail instead of warning, one can run tests specyfing the conf explicitly as cargo test --features "optimizer_failed_rules"

Hi @comphead. Apologies for the delay in reviewing. I'm not sure that we want to introduce feature flags for this. I was thinking that we could explicitly set datafusion.optimizer.skip_failed_rules in tests. The goal would be to eventually set this to false but for some tests we would need to leave it as true for now and then file issues for fixing those tests. Does that make sense?

comphead · 2022-10-27T19:57:43Z

@andygrove got your point thanks. Please check renewed PR
So idea is by default tests run with enabled skip_optimization_errors, to make them fail instead of warning, one can run tests specyfing the conf explicitly as cargo test --features "optimizer_failed_rules"
Hi @comphead. Apologies for the delay in reviewing. I'm not sure that we want to introduce feature flags for this. I was thinking that we could explicitly set datafusion.optimizer.skip_failed_rules in tests. The goal would be to eventually set this to false but for some tests we would need to leave it as true for now and then file issues for fixing those tests. Does that make sense?

Thanks @andygrove for feedback.

Please correct me if I'm wrong.

We have specific test list that should be fixed later but now works not fully optimized, and we ok, therefore these tests require optimizer_skip_error as true. And this test list is known.
For all other tests(existing or new) optimizer_skip_error has to be false.

If thats correct, I will rework the PR

andygrove · 2022-11-17T16:20:36Z

Now that #4208 is implemented, I suggest that we:

Update existing rules to implement try_optimize so that they return Ok(None) instead of Err when the rule does not support the plan (we should create one PR per rule)
Change the default value of datafusion.optimizer.skip_failed_rules to false, and fix any test failures

comphead · 2022-11-17T17:30:34Z

Ok

Thanks for letting me know, checking this.

skip failing tests default to false
e487589

github-actionsBot added the core Core DataFusion crate label Oct 19, 2022

comphead added 2 commits October 18, 2022 17:26

fix test name
39ecfd3

Merge branch 'master' into skip_failing_tests_true
c70dd4f
# Conflicts: # datafusion/core/src/config.rs

alamb previously approved these changes Oct 19, 2022
View reviewed changes

alamb reviewed Oct 19, 2022
View reviewed changes

andygrove requested changes Oct 19, 2022
View reviewed changes

make regression tests to run as cfg
21712d0

github-actionsBot added logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates sql SQL Planner labels Oct 22, 2022

cargo toml fmt
45e8c0c

alamb marked this pull request as draft October 27, 2022 14:27

comphead closed this Mar 24, 2023

	ConfigDefinition::new_bool(
	// skip_failing_rules default to true in tests so that regressions are caught much sooner
	ConfigDefinition::new_bool(

Uh oh!

Conversation

comphead commented Oct 19, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

comphead commented Oct 19, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alambOct 19, 2022

Choose a reason for hiding this comment

Uh oh!

alambOct 19, 2022

Choose a reason for hiding this comment

Uh oh!

alambOct 19, 2022

Choose a reason for hiding this comment

Uh oh!

alambOct 19, 2022

Choose a reason for hiding this comment

Uh oh!

alambOct 19, 2022

Choose a reason for hiding this comment

Uh oh!

comphead commented Oct 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andygrove commented Oct 19, 2022

Uh oh!

alamb commented Oct 19, 2022

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

comphead commented Oct 19, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comphead commented Oct 20, 2022

Uh oh!

andygrove commented Oct 21, 2022

Uh oh!

comphead commented Oct 22, 2022

Uh oh!

comphead commented Oct 22, 2022

Uh oh!

comphead commented Oct 25, 2022

Uh oh!

alamb commented Oct 27, 2022

Uh oh!

andygrove commented Oct 27, 2022

Uh oh!

comphead commented Oct 27, 2022

Uh oh!

andygrove commented Nov 17, 2022

Uh oh!

comphead commented Nov 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

comphead commented Oct 19, 2022•
edited
Loading

comphead commented Oct 19, 2022•
edited
Loading