-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix explain plan formatting in sqllogictest #6329
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
Conversation
physical_plan | ||
ProjectionExec: expr=[c9@0 as c9, FIRST_VALUE(aggregate_test_100.c9) ORDER BY [aggregate_test_100.c9 ASC NULLS LAST] ROWS BETWEEN 1 PRECEDING AND 5 FOLLOWING@4 as fv1, FIRST_VALUE(aggregate_test_100.c9) ORDER BY [aggregate_test_100.c9 DESC NULLS FIRST] ROWS BETWEEN 1 PRECEDING AND 5 FOLLOWING@1 as fv2, LAG(aggregate_test_100.c9,Int64(2),Int64(10101)) ORDER BY [aggregate_test_100.c9 ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW@5 as lag1, LAG(aggregate_test_100.c9,Int64(2),Int64(10101)) ORDER BY [aggregate_test_100.c9 DESC NULLS FIRST] ROWS BETWEEN 10 PRECEDING AND 1 FOLLOWING@2 as lag2, LEAD(aggregate_test_100.c9,Int64(2),Int64(10101)) ORDER BY [aggregate_test_100.c9 ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW@6 as lead1, LEAD(aggregate_test_100.c9,Int64(2),Int64(10101)) ORDER BY [aggregate_test_100.c9 DESC NULLS FIRST] ROWS BETWEEN 10 PRECEDING AND 1 FOLLOWING@3 as lead2] |
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.
Here is an example I think that shows the indent being properly displayed
I have reviewed the PR. This PR is LGTM!. Now the final plan is more clear, especially when the query involves executors with multiple children (such as union, join, etc.). Previously there were test cases, an executor which has seemingly multiple children (because of indentation), whereas it has indeed single children. This PR fixes these kind of confusions. @alamb thanks for this improvement. |
During experimenting, I have tried to mess indentation and run the tests. Tests still pass. For instance both |ProjectionExec: expr=[c9@0 as c9, SUM(aggregate_test_100.c9) ORDER BY [aggregate_test_100.c9 ASC NULLS LAST] ROWS BETWEEN 1 PRECEDING AND 5 FOLLOWING@2 as sum1, SUM(aggregate_test_100.c9) ORDER BY [aggregate_test_100.c9 DESC NULLS FIRST] ROWS BETWEEN 1 PRECEDING AND 5 FOLLOWING@1 as sum2]
| GlobalLimitExec: skip=0, fetch=5
| BoundedWindowAggExec: wdw=[SUM(aggregate_test_100.c9): Ok(Field { name: "SUM(aggregate_test_100.c9)", data_type: UInt64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(5)), end_bound: Following(UInt64(1)) }], mode=[Sorted]
| BoundedWindowAggExec: wdw=[SUM(aggregate_test_100.c9): Ok(Field { name: "SUM(aggregate_test_100.c9)", data_type: UInt64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(1)), end_bound: Following(UInt64(5)) }], mode=[Sorted]
| SortExec: expr=[c9@0 DESC]
| CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c9], has_header=true and |ProjectionExec: expr=[c9@0 as c9, SUM(aggregate_test_100.c9) ORDER BY [aggregate_test_100.c9 ASC NULLS LAST] ROWS BETWEEN 1 PRECEDING AND 5 FOLLOWING@2 as sum1, SUM(aggregate_test_100.c9) ORDER BY [aggregate_test_100.c9 DESC NULLS FIRST] ROWS BETWEEN 1 PRECEDING AND 5 FOLLOWING@1 as sum2]
| GlobalLimitExec: skip=0, fetch=5
| BoundedWindowAggExec: wdw=[SUM(aggregate_test_100.c9): Ok(Field { name: "SUM(aggregate_test_100.c9)", data_type: UInt64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(5)), end_bound: Following(UInt64(1)) }], mode=[Sorted]
| BoundedWindowAggExec: wdw=[SUM(aggregate_test_100.c9): Ok(Field { name: "SUM(aggregate_test_100.c9)", data_type: UInt64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(1)), end_bound: Following(UInt64(5)) }], mode=[Sorted]
| SortExec: expr=[c9@0 DESC]
| CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/testing/data/csv/aggregate_test_100.csv]]}, projection=[c9], has_header=true passes from the test. I guess still, we ignore white spaces at the beginning. If one pastes the result produced at the failing tests, their formatting is right. Hence if a person copy pastes actual result of the test, we will not see this behaviour. However, in the long run, if we can enforce the indentation level for the tests, it would be great. It is not necessarily this PR's concern. We can do so as a follow up Pr. |
Hmm, this seems pretty bad as if the plan changes the tests won't fail. Let me see if I can find something that looks better |
@mustafasrepo I updated the code to replace leading spaces with Now when I mess up the indent (remove a What do you think? |
This is great!. Now we can make sure that indentation level is exact. Thanks for this fix. |
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.
Not related for this PR but rather than for sqllogic improvement.
Sqllogic tests doesn't cover cases if the column name is not expected.
The same relates to datatype, but this can be solved by extra arrow_typeof
Yes, I agree this is not currently covered by the sqllogictest -- I'll file a ticket to try to work on this (#6349)
SQLlogictests does verify the (sql) types of the output (or example, the
|
Which issue does this PR close?
Closes #6328 (noticed by @mustafasrepo on #6234)
Rationale for this change
For some reason,
cargo test --test sqllogictests -- --complete
when completing tests strips the leading whitespace sometimes. Then the comparison also ignores leading whitespaceSo sometimes the explain plans captured by
--compete
would take a plan like:And display it like:
What changes are included in this PR?
sqlogictest
to prefix explain plan lines with|
~sqlogictest
to replace leading spaces with-
cargo test --test sqllogictests -- --complete
Are these changes tested?
Yes
Are there any user-facing changes?
no