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

Allow order by on non-indexed fields #1961

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

ostafen
Copy link
Collaborator

@ostafen ostafen commented Apr 15, 2024

This merge request is meant to support the order by clause on non indexed fields.
If the query planner determines that the output is not already sorted, then an additional sorting step is added through a new type of reader: sortReader.
When the client attempts to read the first row on the sort reader, all rows gets fetched from the underlying reader and collected to a buffer. When the buffer contains at least 1024 rows, sorting is performed in memory and the sorted chunk is spilled to a temporary disk file. The process continues until all the result rows have been read.
At this point, sorted disk chunks (if any) are merged into a single bigger sorted chunk and a reader to such a file is returned to the client. If the output contains a number of rows <= 1024, then a reader to the buffer is simply returned.

@coveralls
Copy link
Collaborator

coveralls commented Apr 15, 2024

Coverage Status

coverage: 89.421% (-0.04%) from 89.459%
when pulling a2d377a on ostafen:feature/filesort
into e9e5c47 on codenotary:master.

@ostafen ostafen marked this pull request as ready for review April 16, 2024 15:28
@ostafen ostafen force-pushed the feature/filesort branch 2 times, most recently from d884c10 to 3026ce1 Compare April 17, 2024 09:32
@ostafen ostafen requested a review from Razikus April 17, 2024 09:51
Copy link
Contributor

@Razikus Razikus left a comment

Choose a reason for hiding this comment

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

do we have some small benchmark for this PR? just to know if there are implications for performance

embedded/sql/sql_tx.go Outdated Show resolved Hide resolved
@@ -75,3 +80,8 @@ func (opts *Options) WithMultiDBHandler(multidbHandler MultiDBHandler) *Options
opts.multidbHandler = multidbHandler
return opts
}

func (opts *Options) WithSortBufferSize(size int) *Options {
opts.sortBufferSize = size
Copy link
Contributor

Choose a reason for hiding this comment

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

we should document it - what it is, what will increase of this value provide, what if we will set to 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added validity check and description

}

if sortingIndex == nil {
return nil, ErrNoAvailableIndex
Copy link
Contributor

Choose a reason for hiding this comment

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

this could potentially affects current users code, should be communicated somehow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Razikus
Razikus previously approved these changes Apr 17, 2024
Razikus
Razikus previously approved these changes Apr 18, 2024
@ostafen ostafen merged commit 68b32d5 into codenotary:master Apr 26, 2024
17 checks passed
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