Skip to content
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

Use ordinals to force stable TopN heap sort #1690

Merged
merged 3 commits into from
Apr 4, 2023
Merged

Conversation

max-hoffman
Copy link
Contributor

@max-hoffman max-hoffman commented Apr 3, 2023

Re: dolthub/dolt#5640

... ORDER BY <expr> LIMIT 1 and ... ORDER BY <expr> LIMIT 1 OFFSET 1 both use TopN in a way that surprises users expecting stable output orders.

For example, the second query returning the first row is within the SQL spec but not user friendly:

-- setup
create table xy (x int primary key, y int);
insert into xy values (1,0),(2,0),(3,0),(4,0);
-- expect first row
select * from xy order by y asc limit 1;
+---+------+
| x | y    |
+---+------+
| 0 |    1 |
+---+------+
-- expect second row
select * from xy order by y asc limit 1 offset 1;
+---+------+
| x | y    |
+---+------+
| 0 |    1 |
+---+------+

This PR adds ordinals to rows in the TopN heap to differentiate insertion order in sort-groupings.

@max-hoffman max-hoffman requested review from zachmu and reltuk and removed request for zachmu April 3, 2023 20:38
sql/plan/sort.go Outdated
@@ -397,9 +399,9 @@ type topRowsIter struct {
idx int
}

func newTopRowsIter(s sql.SortFields, limit int64, calcFoundRows bool, child sql.RowIter) *topRowsIter {
func newTopRowsIter(s sql.SortFields, limit int64, calcFoundRows bool, child sql.RowIter, childLen int) *topRowsIter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: rename childLen to childSchemaLen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Contributor

@reltuk reltuk left a comment

Choose a reason for hiding this comment

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

LGTM!

@max-hoffman max-hoffman merged commit dddc9ed into main Apr 4, 2023
@max-hoffman max-hoffman deleted the max/stable-topn branch April 4, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants