-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Disable skip_failed_rules
optimizer config by default
#6265
Conversation
skip_failed_rules
optimizer rules by default
skip_failed_rules
optimizer rules by defaultskip_failed_rules
optimizer config by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jackwener ❤️
@@ -576,6 +576,8 @@ async fn support_order_by_correlated_columns() -> Result<()> { | |||
Ok(()) | |||
} | |||
|
|||
// TODO: issue https://github.com/apache/arrow-datafusion/issues/6263 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 so only 1 test gets failed on optimization rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which rule fail this test ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which rule fail this test ?
decorrelate_where_in, you can see issue #6263
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix this rule later.
This is a I prepare to wait for more opinion for it. |
Given how many bugs and confusion this rule has caused I strongly support this behavior change -- the fact we have a workaround (to turn on the setting) is great. I think waiting for more opinions is a great idea, BTW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
we should not ignore any error, it will hide some bugs.
Thanks everyone ❤️ |
* Disable skip_failed_rules default * fix slt
Which issue does this PR close?
Closes #4615.
Rationale for this change
#4685
What changes are included in this PR?
Disable skip_failed_rules default
Are these changes tested?
Are there any user-facing changes?
YES!!!
This PR change default behavior.