Minor: Begin porting some window tests to sqllogictests #5199
Conversation
github-actions
bot
added
core
|
There is something wrong with the plan diff comparison -- will review shortly |
alamb
force-pushed
the
alamb/port_window_tests
branch
from
be315fe
to
0248154
Compare
alamb
force-pushed
the
alamb/port_window_tests
branch
from
0248154
to
fd5f325
Compare
| 0 0 220 44 10 0 5 | ||
| 0 1 220 44 10 0 5 | ||
| 0 2 220 44 10 0 5 | ||
| 0 3 220 44 10 0 5 | ||
| 0 4 220 44 10 0 5 |
There was a problem hiding this comment.
For the tests that use partitioned data e.g table test. Results are different than the previous version. I guess the reason for that is that previously first row was not used in the calculations (Maybe there were parsed as header.) I think, the previous results were wrong. Is this so? What do you think about it?
There was a problem hiding this comment.
Maybe there were parsed as header.)
Yes I think this is exactly what was happening -- that is a good observation
I think it is ok that the results changed as the point of the test seems to be to cover a broad range of window function results rather than any specific window function or case (as in skipping the header row seems like a oversight in the original test rather than an important behavior to replicate)
|
Thanks @alamb for this PR. LGTM!. |
| ORDER BY c1, c2 | ||
| LIMIT 5 | ||
| ---- | ||
| 0 0 1 0 0 0 0 1 0 0 0 |
There was a problem hiding this comment.
the careful reader will note that the original test does not have any data for c2 = 0 (because I think it uses a slightly different test setup)
| 0 0 220 44 10 0 5 | ||
| 0 1 220 44 10 0 5 | ||
| 0 2 220 44 10 0 5 | ||
| 0 3 220 44 10 0 5 | ||
| 0 4 220 44 10 0 5 |
There was a problem hiding this comment.
Maybe there were parsed as header.)
Yes I think this is exactly what was happening -- that is a good observation
I think it is ok that the results changed as the point of the test seems to be to cover a broad range of window function results rather than any specific window function or case (as in skipping the header row seems like a oversight in the original test rather than an important behavior to replicate)
|
Benchmark runs are scheduled for baseline = 9565887 and contender = 98dba2b. 98dba2b is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #4495
Rationale for this change
While reviewing #5171 I realized that many of the tests are still written directly in rust, which is not bad but figured I would start porting them to sqllogictests so we can avoid adding to the porting work
I think sqllogictests are easier to write (less boilerplate setup) and maintain (no rust recompilation to update output files). This is also the first sqllogictest I know of that has expain plans, which is exciting
What changes are included in this PR?
Port the first few tests from window.rs to sqllogictests
Are these changes tested?
Yes, they are all tests
Are there any user-facing changes?
No