Skip to content

Analyzer Planner fixes before enable by default#47101

Merged
kitaisreal merged 18 commits intoClickHouse:masterfrom
kitaisreal:analyzer-planner-fixes-before-enable-by-default
Mar 2, 2023
Merged

Analyzer Planner fixes before enable by default#47101
kitaisreal merged 18 commits intoClickHouse:masterfrom
kitaisreal:analyzer-planner-fixes-before-enable-by-default

Conversation

@kitaisreal
Copy link
Copy Markdown
Contributor

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 1, 2023
@kitaisreal kitaisreal added the can be tested Allows running workflows for external contributors label Mar 1, 2023
@novikd novikd self-assigned this Mar 2, 2023
@kitaisreal kitaisreal merged commit 3cd8800 into ClickHouse:master Mar 2, 2023
@@ -1,2 +1,2 @@
1 2 3
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ey @kitaisreal. For these kind of tests where the column order changed, wouldn't it make more sense to either output in a format with columns or force a column order in the query so that the output works with both the analyzer and without it. Otherwise it's really hard to see what things are changed with its introduction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It make sense to me to rewrite test as less as we can, without changing .reference files, so in some particular cases like this when test is small enought and results are expected we can do this. Feel free to help, if you want to remove SET allow_experimental_analyzer = 1; and just rewrite test, pull requests are welcome.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

2 questions:

  • I see that Analyzer Planner enable by default #45461 has some files under utils detailing some test changes and reasons. I assume the behaviour change (aside from changing error codes) is intended right?
  • Is it safe to assume that any old test (old = before the introduction of the analyzer) that has the new analyzer tag has a behaviour that's considered good?

We can send some PRs trying to help in the process and trying to adapt those tests to pass both with and without the analyzer. In my ideal and impossible world, any test that's not specific to the analyzer or new features built on top should work with and without allow_experimental_analyzer. I know that might not be possible, but the less tests with changes the safer I feel at night 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This files only describe tests that are currently failing in new analyzer, and reasons why they fail. Such tests need to be fixed changing new infrastructure a bit. Feel free to send pull requests that fix tests in analyzer, from this file https://github.com/ClickHouse/ClickHouse/pull/45461/files#diff-be63cacb6e7d236ee345ee36890dc6142daa5423415ca0c2cec9963ba963bb66R4, we will appreciate additional help.
I will also appreciate pull requests that fix tests with allow_experimental_analyzer, so it will work in both and new analyzer. I expect that some tests can be fixed manually, so it will be easier to track where analyzer infrastructure actually changed something.
If we manually put allow_experimental_analyzer = 1, and regenerate .reference file, that means we think that new behaviour is right. It is not possible to fix all tests to run in new and old analyzer, because in old infrastructure there are tests with JOINS that are just invalid from SQL correctness, tests with constants in TOTALS, and a couple of others that are just wrong.
In some cases we add flag for backward compatibility like single_join_prefer_left_table, it is invalid SQL, but otherwise it will break too much tests.

@kitaisreal kitaisreal deleted the analyzer-planner-fixes-before-enable-by-default branch March 8, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants