Skip to content

Commit 4bfd13b

Browse files
committed
fix: topK sorted nulls first while full sort path sorted nulls last
The topK heap comparator treated null as "less than" everything (null sorted first), but rowComparator used by the full sort path sorts nulls last. This caused ORDER BY + LIMIT queries to produce different null ordering depending on whether the limit was small enough to trigger the topK path. Fixed topK to treat nulls as "greatest" (nulls-last), matching rowComparator and SQL standard NULLS LAST default.
1 parent 3f64917 commit 4bfd13b

File tree

2 files changed

+57
-6
lines changed

2 files changed

+57
-6
lines changed

src/decode.test.ts

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { describe, it, expect } from "vitest";
2-
import { decodePage, assembleRows, canSkipPage, canSkipFragment, matchesFilter, bigIntReplacer } from "./decode.js";
2+
import { decodePage, assembleRows, applySortAndLimit, canSkipPage, canSkipFragment, matchesFilter, bigIntReplacer } from "./decode.js";
33
import type { ColumnMeta, PageInfo } from "./types.js";
44
import type { QueryDescriptor } from "./client.js";
55
import type { WasmEngine } from "./wasm-engine.js";
@@ -442,6 +442,56 @@ describe("assembleRows", () => {
442442
});
443443
});
444444

445+
describe("applySortAndLimit (topK null ordering)", () => {
446+
it("sorts nulls last in ascending topK", () => {
447+
const rows = [
448+
{ v: null }, { v: 3 }, { v: 1 }, { v: null }, { v: 2 }, { v: 5 }, { v: 4 },
449+
];
450+
const query: QueryDescriptor = {
451+
table: "t", filters: [], projections: ["v"],
452+
sortColumn: "v", sortDirection: "asc", limit: 3,
453+
};
454+
const result = applySortAndLimit(rows, query);
455+
expect(result.map(r => r.v)).toEqual([1, 2, 3]);
456+
});
457+
458+
it("sorts nulls last in descending topK", () => {
459+
const rows = [
460+
{ v: null }, { v: 3 }, { v: 1 }, { v: null }, { v: 2 }, { v: 5 }, { v: 4 },
461+
];
462+
const query: QueryDescriptor = {
463+
table: "t", filters: [], projections: ["v"],
464+
sortColumn: "v", sortDirection: "desc", limit: 3,
465+
};
466+
const result = applySortAndLimit(rows, query);
467+
expect(result.map(r => r.v)).toEqual([5, 4, 3]);
468+
});
469+
470+
it("nulls appear at end when limit exceeds non-null count", () => {
471+
const rows = [
472+
{ v: null }, { v: 2 }, { v: 1 },
473+
];
474+
const query: QueryDescriptor = {
475+
table: "t", filters: [], projections: ["v"],
476+
sortColumn: "v", sortDirection: "asc", limit: 3,
477+
};
478+
const result = applySortAndLimit(rows, query);
479+
expect(result.map(r => r.v)).toEqual([1, 2, null]);
480+
});
481+
482+
it("full sort path also puts nulls last", () => {
483+
const rows = [
484+
{ v: null }, { v: 3 }, { v: 1 }, { v: null }, { v: 2 },
485+
];
486+
const query: QueryDescriptor = {
487+
table: "t", filters: [], projections: ["v"],
488+
sortColumn: "v", sortDirection: "asc",
489+
};
490+
const result = applySortAndLimit(rows, query);
491+
expect(result.map(r => r.v)).toEqual([1, 2, 3, null, null]);
492+
});
493+
});
494+
445495
describe("matchesFilter", () => {
446496
it("matches eq", () => {
447497
expect(matchesFilter(42, { column: "x", op: "eq", value: 42 })).toBe(true);

src/decode.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -359,17 +359,18 @@ function topK(rows: Row[], k: number, col: string, desc: boolean): Row[] {
359359

360360
const cmp = (a: Row, b: Row): number => {
361361
const av = a[col], bv = b[col];
362-
if (av === null && bv === null) return 0;
363-
if (av === null) return -1;
364-
if (bv === null) return 1;
362+
// Nulls-last: null is "greatest" so it sits at the heap root and gets replaced first
363+
if ((av === null || av === undefined) && (bv === null || bv === undefined)) return 0;
364+
if (av === null || av === undefined) return 1;
365+
if (bv === null || bv === undefined) return -1;
365366
const c = av < bv ? -1 : av > bv ? 1 : 0;
366367
return desc ? -c : c;
367368
};
368369

369370
const shouldReplace = (row: Row): boolean => {
370371
const nv = row[col], rv = heap[0][col];
371-
if (nv === null) return false;
372-
if (rv === null) return true;
372+
if (nv === null || nv === undefined) return false;
373+
if (rv === null || rv === undefined) return true;
373374
return desc ? nv > rv : nv < rv;
374375
};
375376

0 commit comments

Comments
 (0)