-
Notifications
You must be signed in to change notification settings - Fork 209
Return error when ORDER BY references unknown column#1073
Conversation
CLA assistant check
All committers have signed the CLA.
@sgrif
sgrif
left a comment
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.
This has no tests. I'm fairly certain there is no case where we would trigger this error where the shards would not have errored resolving the query. Can you provide an example?
Akash504-ai
commented
Jun 15, 2026
Thanks for taking a look. I picked this up because of the existing TODOs in Buffer::sort() that say it should error instead of silently not sorting. My assumption was that if a column name couldn't be resolved from the row description, returning an error would be better than silently skipping the sort.
That said, I don't currently have a real query that would hit this path. The tests I added only cover the buffer logic directly.
If these branches are effectively unreachable because query validation happens earlier, then my assumption was probably wrong. If so, I'm happy to close the PR.
sgrif
commented
Jun 15, 2026
I'm fine with us verifying the invariant and erroring if it's not upheld. But if we expect this code path to never be hit unless there's a bug in pgdog we should have the error reflect that. We shouldn't add a new error variant that implies this is user error
Summary
Replace the silent no-op behavior when resolving named ORDER BY columns fails.
Previously,
Buffer::sort()would silently skip sorting when anORDER BYcolumn name could not be found in the row description. This change returns an explicit error instead.Changes
Add
OrderByColumnNotFound(String)to multi-shard error handling.Make
Buffer::sort()returnResult<(), Error>.Return an error when resolving:
OrderBy::AscColumnOrderBy::DescColumnOrderBy::AscVectorL2Columnfails due to a missing column.
Propagate the error from the multi-shard execution path.
Tests
Added tests covering:
OrderByColumnNotFound.Notes
TODOcomments that indicated missing-column resolution should error instead of silently not sorting.cargo fmtwas run locally.