Skip to content

Return error when ORDER BY references unknown column#1073

Open
Akash504-ai wants to merge 1 commit into
pgdogdev:mainfrom
Akash504-ai:fix-order-by-missing-column-error
Open

Return error when ORDER BY references unknown column#1073
Akash504-ai wants to merge 1 commit into
pgdogdev:mainfrom
Akash504-ai:fix-order-by-missing-column-error

Conversation

@Akash504-ai

Copy link
Copy Markdown

Summary

Replace the silent no-op behavior when resolving named ORDER BY columns fails.

Previously, Buffer::sort() would silently skip sorting when an ORDER BY column 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() return Result<(), Error>.

  • Return an error when resolving:

    • OrderBy::AscColumn
    • OrderBy::DescColumn
    • OrderBy::AscVectorL2Column
      fails due to a missing column.
  • Propagate the error from the multi-shard execution path.

Tests

Added tests covering:

  • Missing named ORDER BY columns return OrderByColumnNotFound.
  • Existing named ORDER BY columns continue to sort correctly.

Notes

  • Replaces the existing TODO comments that indicated missing-column resolution should error instead of silently not sorting.
  • cargo fmt was run locally.

@CLAassistant

CLAassistant commented Jun 13, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@sgrif sgrif left a comment

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Author

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

sgrif commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

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

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