Skip to content

feat: support sort by #819

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: support sort by #819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions proto/substrait/algebra.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ message SortRel {
RelCommon common = 1;
Rel input = 2;
repeated SortField sorts = 3;
bool preserve_partitioning = 4;
substrait.extensions.AdvancedExtension advanced_extension = 10;
}

Expand Down
1 change: 1 addition & 0 deletions site/docs/relations/logical_relations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ The sort operator reorders a dataset based on one or more identified sort fields
| ----------- | ------------------------------------------------------------ | ----------------------- |
| Input | The relational input. | Required |
| Sort Fields | List of one or more fields to sort by. Uses the same properties as the [orderedness](basics.md#orderedness) property. | One sort field required |
| Preserve Partitioning | False means global sorting apply for entire data set, True means sorting only apply within the partition. | Optional |
Copy link
Contributor

@yongchul yongchul Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely against this but two things.

  1. This is physical stuff and we are mixing logical and physical concept. I am closer to purist (keeping logical and physical separate) but if you interpret "physical" sort on particular distribution, you don't need this field. If you want global sort, you can do it over serial (or singleton) partition. If you want distributed global sort, you can do it by running sort over global range distributed partition (i.e., sort on top of global range distributed). So as long as it does not change the distribution, this field does not matter. If you have a logical plan, well, the distribution does not exist so it is global sort.
  2. Should this be partition or distribution? The existing documentation all refer distribution so I suggest to use distribution for consistency.

So, if you can reason about the distribution, why do we need this new field? If this is even for the local parallelism (i.e., thread parallelism), you probably want to keep that orthogonal. So as long as sort does not and should not change any parallelism, distribution, I don't see a strong reason why we need this extra field.

A good example would be helpful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry，maybe this change is not the best solution， but i dont think it's physical stuff， in hive/spark， “select * from tb order by col” “select * from tb sort by col” have totally different meaning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I didn't know about SORT BY. I'd say it's still physical or exposing the implementation detail of execution because sort by order is inherently tied to the number of reducers and the way the query result is constructed (i.e., what is the order of the sort by? without understanding of how the result is laid out, you can not define the order).

Anyway, I still think it is better to handled with proper distribution and keep the distribution and order orthorgonal. In that regard, sort by already assuming that you are generating some sort of distributed result and you don't really care about the distribution. I'm not sure whether that can be expressed in "logical" sense today (need to tinker on it).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we use sort by, we don't care about how the result is globally laid out. we won't use sort by in subquery, most time we use it with create table. it's used to optimize output files. with this operator, we can make output files much smaller than unsorted files, and we can also easily build index on each partition. when we query this table next time, it's much faster than unsorted one.

Copy link
Contributor

@yongchul yongchul Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There you said create table and partition :-) The partition is how the result is laid out, physically and globally, but the sort by does not explicitly saying what those are -- just whatever assumed by the implementation.

As I said in the beginning, I'm not against the change -- modulo terms and how we represent this. The real distinction is whether the sort is a global sort (total order but still can be distributed) or not. I prefer local sort for this (heavy-bias to the systems that I worked on).


=== "SortRel Message"

Expand Down