Analyzer Planner fixes before enable by default#47101
Analyzer Planner fixes before enable by default#47101kitaisreal merged 18 commits intoClickHouse:masterfrom
Conversation
| @@ -1,2 +1,2 @@ | |||
| 1 2 3 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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.
Changelog category (leave one):