Skip to content

Conversation

@jayzhan211
Copy link
Contributor

@jayzhan211 jayzhan211 commented Mar 19, 2025

Which issue does this PR close?

This CI failure is caused by the conflict with #15135, so is not detected

  • Closes #.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 19, 2025
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.
Copy link
Contributor Author

@jayzhan211 jayzhan211 Mar 19, 2025

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 🤔

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@jayzhan211 jayzhan211 changed the title Fix union in view table test CI Red: Fix union in view table test Mar 19, 2025
@jayzhan211 jayzhan211 marked this pull request as ready for review March 19, 2025 01:17
@jayzhan211 jayzhan211 requested a review from jonahgao March 19, 2025 02:22
Copy link
Member

@jonahgao jonahgao left a 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.
Copy link
Member

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.

@jonahgao jonahgao merged commit 6c5c5c1 into apache:main Mar 19, 2025
29 checks passed
@jayzhan211 jayzhan211 deleted the view-sort branch March 19, 2025 02:49

# optimize_subquery_sort in create_relation removes Sort so the result is not sorted.
query I
SELECT * FROM v1;
Copy link
Contributor

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

Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants