test(order_by): add ORE ordering test without WHERE clause operators#168
Merged
test(order_by): add ORE ordering test without WHERE clause operators#168
Conversation
Verifies ORDER BY eql_v2.order_by(e) DESC works over the full dataset without relying on any comparison operators in a WHERE clause.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces in-test setup with fixture-driven data for ORDER BY NULL tests, adds fixtures that drop specific eql_v2 operator classes, introduces multiple new test modules verifying ORDER BY behavior and failure modes when opclasses/operators are absent, and adds SQL sorting utilities plus two new helper-ordering tests without WHERE. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…y workaround Add fixture that drops operator classes/operators to simulate Supabase environment. Tests prove ORDER BY e and ORDER BY eql_v2.order_by(e) both produce wrong results without operator classes. Add correlated subquery ranking tests demonstrating eql_v2.compare() as a working alternative for correct ordering in Supabase mode: - ASC/DESC full ordering verification - LIMIT support - WHERE clause filtering with correct sort order
Verify ORDER BY e USING </> syntax works with raw operators independently of operator class registration. Tests ascending, descending, LIMIT, and WHERE clause filtering after dropping btree/hash operator families.
- Extract shared NULL test data into order_by_null_data.sql fixture, removing ~120 lines of duplicated setup across 4 tests - Deduplicate correlated subquery WHERE test (single fetch_all instead of QueryAssertion + fetch_one running the query twice) - Add ASC variant for order_by helper without WHERE clause - Remove redundant USING+LIMIT tests (same plan-time error as non-LIMIT) - Document new fixture in FIXTURE_SCHEMA.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/sqlx/fixtures/order_by_null_data.sql (1)
13-30: Add a small validation test for this fixture.Lines 24 and 27 can silently insert zero rows if the seeded
oredata ever changes, which would turn the NULL-ordering tests into fixture-debugging exercises. A dedicated validation test for the expected 4-row shape would make this fixture much safer to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sqlx/fixtures/order_by_null_data.sql` around lines 13 - 30, Add a small validation test that runs after this fixture to assert the encrypted table has exactly four rows so missing INSERTs don’t silently break NULL-order tests: write a test that queries SELECT count(*) FROM encrypted and asserts the result equals 4 (and optionally checks that rows with id=1 and id=4 have NULL e and id=2 and id=3 have non-NULL e) to fail fast if the ore-derived INSERTs (the SELECT e FROM ore WHERE id = 42 / id = 3 lines) insert zero rows; add this test to the fixture’s test suite so it runs immediately after loading the SQL fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/sqlx/tests/order_by_using_operator_tests.rs`:
- Around line 18-25: The test currently only checks result.is_err(), which
accepts any error; replace that with extracting the error (e.g., let err =
result.unwrap_err()) and assert the specific failure by matching the SQL error
code or message for missing btree ordering support (for the query string "SELECT
id FROM ore ORDER BY e USING <" and the other similar queries in the same file),
e.g., assert that err contains the expected SQLSTATE or a substring like
"operator family" or "could not identify an ordering operator family", and apply
the same tightened assertion to the other cases referenced (lines 34-41 and
52-64).
---
Nitpick comments:
In `@tests/sqlx/fixtures/order_by_null_data.sql`:
- Around line 13-30: Add a small validation test that runs after this fixture to
assert the encrypted table has exactly four rows so missing INSERTs don’t
silently break NULL-order tests: write a test that queries SELECT count(*) FROM
encrypted and asserts the result equals 4 (and optionally checks that rows with
id=1 and id=4 have NULL e and id=2 and id=3 have non-NULL e) to fail fast if the
ore-derived INSERTs (the SELECT e FROM ore WHERE id = 42 / id = 3 lines) insert
zero rows; add this test to the fixture’s test suite so it runs immediately
after loading the SQL fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0dde4a5-da4f-4d9e-b1e5-de97f12cdb81
📒 Files selected for processing (5)
tests/sqlx/fixtures/FIXTURE_SCHEMA.mdtests/sqlx/fixtures/order_by_null_data.sqltests/sqlx/tests/order_by_no_opclass_tests.rstests/sqlx/tests/order_by_tests.rstests/sqlx/tests/order_by_using_operator_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/sqlx/tests/order_by_no_opclass_tests.rs
Comment on lines
+18
to
+25
| let result = sqlx::query("SELECT id FROM ore ORDER BY e USING <") | ||
| .fetch_all(&pool) | ||
| .await; | ||
|
|
||
| assert!( | ||
| result.is_err(), | ||
| "ORDER BY e USING < should fail without btree operator family" | ||
| ); |
There was a problem hiding this comment.
Assert the specific failure, not just is_err().
These tests currently pass on any error path, so they do not prove that ORDER BY ... USING is failing because the btree ordering support is missing. In particular, the third case would also pass on an unrelated SQL/literal issue. Please check the returned error message or SQLSTATE for the expected ordering-operator failure.
🔎 Tighten one of the assertions
- let result = sqlx::query("SELECT id FROM ore ORDER BY e USING <")
- .fetch_all(&pool)
- .await;
-
- assert!(
- result.is_err(),
- "ORDER BY e USING < should fail without btree operator family"
- );
+ let err = sqlx::query("SELECT id FROM ore ORDER BY e USING <")
+ .fetch_all(&pool)
+ .await
+ .expect_err("ORDER BY e USING < should fail without btree operator family");
+
+ let msg = err.to_string();
+ assert!(
+ msg.contains("valid ordering operator") || msg.contains("operator family"),
+ "expected missing btree ordering support error, got: {msg}"
+ );Also applies to: 34-41, 52-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/sqlx/tests/order_by_using_operator_tests.rs` around lines 18 - 25, The
test currently only checks result.is_err(), which accepts any error; replace
that with extracting the error (e.g., let err = result.unwrap_err()) and assert
the specific failure by matching the SQL error code or message for missing btree
ordering support (for the query string "SELECT id FROM ore ORDER BY e USING <"
and the other similar queries in the same file), e.g., assert that err contains
the expected SQLSTATE or a substring like "operator family" or "could not
identify an ordering operator family", and apply the same tightened assertion to
the other cases referenced (lines 34-41 and 52-64).
freshtonic
approved these changes
Mar 11, 2026
Contributor
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
This works but via a technique with quadratic time complexity.
I've found no way to convince Postgres to order with a comparison function without custom types/operator classes.
However, if we dropped the original block ORE schema and only provide the lexicographical ORE variant then this problem would go away entirely.
Comment on lines
+177
to
+178
| let sql = "SELECT id FROM ore t \ | ||
| ORDER BY (SELECT COUNT(*) FROM ore t2 WHERE eql_v2.compare(t.e, t2.e) > 0)"; |
Contributor
There was a problem hiding this comment.
While this works around the issue it does have quadratic time complexity - O(N2) - because the subquery generates the ranking across the whole table for every row in the table.
Not something that should be fixed in this PR necessarily, but we should circle back to figure out something more optimal.
…rows Expand the 99-row ORE dataset to 495 rows via generate_series cross join to demonstrate O(n log n) vs O(n^2) divergence more dramatically. At 495 rows, sort_compare achieves ~28x speedup over correlated subquery. Also adds sort.sql with the quicksort implementation (sort_compare and order_by_compare functions).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/sqlx/tests/order_by_sort_tests.rs (1)
316-322: Performance assertion is defensive but may miss regressions.The assertion
sort_elapsed < correlated_elapsed * 3allowssort_compareto be up to 3x slower than the correlated subquery, which contradicts the test name "faster_than". This is understandable for CI stability, but consider tightening tosort_elapsed < correlated_elapsedwith a retry mechanism or statistical sampling if flakiness becomes an issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sqlx/tests/order_by_sort_tests.rs` around lines 316 - 322, The test currently allows sort_compare to be up to 3x slower via the assertion using sort_elapsed and correlated_elapsed; change the assertion in the test (the block using sort_elapsed and correlated_elapsed in order_by_sort_tests.rs / the "faster_than" test) to require sort_elapsed < correlated_elapsed (strictly faster), and add a simple retry/statistical approach: run the timing measurement multiple times (e.g., N runs), use the median or best-case timing for both sort_elapsed and correlated_elapsed, and only assert on those aggregated values; keep variable names sort_elapsed and correlated_elapsed and ensure retries are bounded and logged to avoid CI flakiness.src/operators/sort.sql (1)
128-146: Consider adding array length validation.The documentation states arrays "must be same length" but there's no runtime check. Mismatched lengths would silently produce incorrect results (sorting stops at the shorter array's length).
🛡️ Add length validation
n := array_length(ids, 1); IF n IS NULL OR n = 0 THEN RETURN; END IF; + + IF n != array_length(vals, 1) THEN + RAISE EXCEPTION 'ids and vals arrays must have the same length'; + END IF; IF n = 1 THEN🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operators/sort.sql` around lines 128 - 146, The function eql_v2.sort_compare currently computes n := array_length(ids,1) but never validates that vals has the same length; add a runtime check after computing n to get m := array_length(vals,1) and if m IS NULL OR n IS NULL OR n <> m then RAISE EXCEPTION with a clear message (e.g., "ids and vals must be non-empty and the same length") so mismatched or nil arrays fail fast; reference ids, vals, n and the function name eql_v2.sort_compare when locating where to insert this validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/operators/sort.sql`:
- Around line 128-146: The function eql_v2.sort_compare currently computes n :=
array_length(ids,1) but never validates that vals has the same length; add a
runtime check after computing n to get m := array_length(vals,1) and if m IS
NULL OR n IS NULL OR n <> m then RAISE EXCEPTION with a clear message (e.g.,
"ids and vals must be non-empty and the same length") so mismatched or nil
arrays fail fast; reference ids, vals, n and the function name
eql_v2.sort_compare when locating where to insert this validation.
In `@tests/sqlx/tests/order_by_sort_tests.rs`:
- Around line 316-322: The test currently allows sort_compare to be up to 3x
slower via the assertion using sort_elapsed and correlated_elapsed; change the
assertion in the test (the block using sort_elapsed and correlated_elapsed in
order_by_sort_tests.rs / the "faster_than" test) to require sort_elapsed <
correlated_elapsed (strictly faster), and add a simple retry/statistical
approach: run the timing measurement multiple times (e.g., N runs), use the
median or best-case timing for both sort_elapsed and correlated_elapsed, and
only assert on those aggregated values; keep variable names sort_elapsed and
correlated_elapsed and ensure retries are bounded and logged to avoid CI
flakiness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e4f32a5-42a5-4a1e-84b6-7a46f91b1c24
📒 Files selected for processing (2)
src/operators/sort.sqltests/sqlx/tests/order_by_sort_tests.rs
Independent array_agg() subqueries don't guarantee matching row order, which could silently misalign ids with ciphertexts. Adding ORDER BY id inside each array_agg() ensures deterministic, aligned array construction. Updates 8 test call sites and 3 docstring examples.
tobyhede
force-pushed
the
expand-order-tests
branch
from
2928cdb to
2b4e0d8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/sqlx/tests/order_by_sort_tests.rs (1)
394-450: Consider adding a small margin to the scaled performance assertion.The assertion at lines 442-447 is stricter than the 99-row test (no margin). While the O(n log n) vs O(n²) gap should be substantial at 495 rows, CI environments can have variable performance. If this test becomes flaky, consider adding a small margin (e.g.,
sort_elapsed * 2 < correlated_elapsed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sqlx/tests/order_by_sort_tests.rs` around lines 394 - 450, The assertion in sort_compare_performance_at_scale is too strict and can be flaky in CI; change the final assertion to require a small margin (e.g., correlated_elapsed is at least twice as slow as sort_elapsed) by comparing their seconds as floats: replace the assert!(sort_elapsed < correlated_elapsed, ...) with an assertion like assert!(correlated_elapsed.as_secs_f64() > sort_elapsed.as_secs_f64() * 2.0, "...") referencing the existing sort_elapsed and correlated_elapsed variables inside the sort_compare_performance_at_scale test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/sqlx/tests/order_by_sort_tests.rs`:
- Around line 394-450: The assertion in sort_compare_performance_at_scale is too
strict and can be flaky in CI; change the final assertion to require a small
margin (e.g., correlated_elapsed is at least twice as slow as sort_elapsed) by
comparing their seconds as floats: replace the assert!(sort_elapsed <
correlated_elapsed, ...) with an assertion like
assert!(correlated_elapsed.as_secs_f64() > sort_elapsed.as_secs_f64() * 2.0,
"...") referencing the existing sort_elapsed and correlated_elapsed variables
inside the sort_compare_performance_at_scale test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd28ca3c-9f7f-45a9-9ad9-0eb5df425c67
📒 Files selected for processing (2)
src/operators/sort.sqltests/sqlx/tests/order_by_sort_tests.rs
✅ Files skipped from review due to trivial changes (1)
- src/operators/sort.sql
Add sort_compare(text, text, text, text, text) overload that accepts column names, table name, optional direction and filter instead of pre-aggregated arrays. Delegates to order_by_compare internally. Includes 8 tests covering ASC/DESC, default direction, LIMIT, filter, empty results, NULL handling, and equivalence with order_by_compare.
Add required Doxygen tags for _compare_sort_pivot, _insertion_sort, and _emit_sorted_rows to pass docs:validate:required-tags.
Expand numeric ORE fixture from 99 to 1000 rows and add text ORE fixture with 100 lexicographically sorted words for broader test coverage. - Expand migration 002 to 1000 numeric ORE rows - Add migration 006 with 100 text ORE rows (aardvark..zinc) - Add get_ore_text_encrypted and assert_sequential_ids helpers - Update all existing test assertions for the expanded dataset - Add 14 new text ORE ordering tests (operator classes, sort_compare, prefix ordering, similar starts edge cases) - Simplify performance tests to use 1000 base rows directly
Add 29 tests covering equality, comparison, function, and JSONB variants for text ORE encryption using ore_text table (100 words). Includes lexicographic edge cases (prefix ordering, similar starts) and boundary tests. Adds get_ore_text_encrypted_as_jsonb helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Verifies ORDER BY eql_v2.order_by(e) DESC works over the full dataset without relying on any comparison operators in a WHERE clause.
Summary by CodeRabbit
New Features
Tests
Documentation