-
Notifications
You must be signed in to change notification settings - Fork 1.5k
CI Red: Fix union in view table test #15300
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
07)----SortExec: expr=[y@0 ASC NULLS LAST], preserve_partitioning=[false] | ||
08)------DataSourceExec: partitions=1, partition_sizes=[1] | ||
|
||
# optimize_subquery_sort in create_relation removes Sort so the result is not sorted. |
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.
I have no idea why we have optimize plan rule optimize_subquery_sort
in create_relation
, I think we should move such rule to optimizer 🤔
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.
Before #15201 - This plan doesn't meet the optimize rule condition because we didn't inline view table at this point, so the Sort is not removed.
Now - The view table inlined so Sort
is removed by this function.
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.
I think removing Sort makes sense, but we need to move this rule to optimizer.
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.
I agree using ORDER BY in a view is meaningless.
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.
LGTM, Thank you @jayzhan211
07)----SortExec: expr=[y@0 ASC NULLS LAST], preserve_partitioning=[false] | ||
08)------DataSourceExec: partitions=1, partition_sizes=[1] | ||
|
||
# optimize_subquery_sort in create_relation removes Sort so the result is not sorted. |
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.
I agree using ORDER BY in a view is meaningless.
07)----SortExec: expr=[y@0 ASC NULLS LAST], preserve_partitioning=[false] | ||
08)------DataSourceExec: partitions=1, partition_sizes=[1] | ||
|
||
# optimize_subquery_sort in create_relation removes Sort so the result is not sorted. | ||
query I | ||
SELECT * FROM v1; |
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.
In this Select query, Postgre does not drop the Sort operation, FYI @jayzhan211 @jonahgao
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.
I think either is fine. A view is a type of relation, and a relation is unordered.
In the SQL Server doc, it states
The ORDER BY clause does not guarantee ordered results when the view is queried, unless ORDER BY is also specified in the query itself.
Postgres also does not drop the Sort operation in subqueries but optimize_subquery_sort does.
psql=> explain select * from (select from u1 order by x);
QUERY PLAN
----------------------------------------------------------------------------
Subquery Scan on unnamed_subquery (cost=158.51..186.76 rows=2260 width=0)
-> Sort (cost=158.51..164.16 rows=2260 width=4)
Sort Key: u1.x
-> Seq Scan on u1 (cost=0.00..32.60 rows=2260 width=4)
(4 rows)
Which issue does this PR close?
This CI failure is caused by the conflict with #15135, so is not detected
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?