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

Disable skip_failed_rules optimizer config by default #6265

Merged
merged 2 commits into from
May 10, 2023

Conversation

jackwener
Copy link
Member

@jackwener jackwener commented May 6, 2023

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.

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels May 6, 2023
@jackwener jackwener requested review from alamb and HaoYang670 May 8, 2023 14:37
@alamb alamb changed the title Disable skip_failed_rules default Disable skip_failed_rules optimizer rules by default May 8, 2023
@alamb alamb changed the title Disable skip_failed_rules optimizer rules by default Disable skip_failed_rules optimizer config by default May 8, 2023
Copy link
Contributor

@alamb alamb left a 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
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

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 ?

Copy link
Member Author

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

Copy link
Contributor

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.

@jackwener
Copy link
Member Author

This is a behavior change PR.

I prepare to wait for more opinion for it.

@alamb
Copy link
Contributor

alamb commented May 8, 2023

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

Copy link
Contributor

@liukun4515 liukun4515 left a 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.

@jackwener
Copy link
Member Author

Thanks everyone ❤️

@jackwener jackwener merged commit 496fc39 into apache:main May 10, 2023
@jackwener jackwener deleted the skip branch May 10, 2023 07:58
Ted-Jiang pushed a commit to Ted-Jiang/arrow-datafusion that referenced this pull request May 11, 2023
* Disable skip_failed_rules default

* fix slt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't ignore failed optimizer rules
5 participants