Skip to content

fix: ORDER BY/window partition preserves prior boundaries on NULL & const keys#24259

Open
aunjgr wants to merge 1 commit intomatrixorigin:mainfrom
aunjgr:fix/order-by-multikey-null
Open

fix: ORDER BY/window partition preserves prior boundaries on NULL & const keys#24259
aunjgr wants to merge 1 commit intomatrixorigin:mainfrom
aunjgr:fix/order-by-multikey-null

Conversation

@aunjgr
Copy link
Copy Markdown
Contributor

@aunjgr aunjgr commented Apr 29, 2026

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #24248

What this PR does / why we need it:

partition.Partition is invoked once per sort key from
pkg/sql/colexec/order/order.go::sortAndSend (and per partition spec
from pkg/sql/colexec/window/window.go), reusing the same diffs
slice across calls. The contract is that each call must OR new
boundaries onto diffs — never overwrite an existing true.

Two code paths in pkg/partition/partition.go violated that contract:

  1. Both-NULL overwrite. In genericPartition / bytesPartition,
    when both adjacent rows were NULL under the current key the code
    did diffs[i] = false, erasing a boundary set by a prior key.
    This made multi-key ORDER BY over a FULL OUTER JOIN produce rows
    out of order: with the primary key NULL on the right-padded side,
    the secondary key was never sub-sorted within the t1.s = NULL
    partition. Repro from the issue:

    create table t1(s int, v varchar(5));
    create table t2(s int, v varchar(5));
    insert into t1 values (1,'a'),(5,'b'),(NULL,'x');
    insert into t2 values (13,'c'),(14,NULL);
    select t1.s, t1.v, t2.s, t2.v
      from t1 full outer join t2 on t1.s = t2.s
      order by t1.s, t2.s, t1.v, t2.v;

    Before this PR, (NULL,NULL,14,NULL) was emitted before
    (NULL,NULL,13,'c'). After: 13 before 14, as expected.

  2. Const-vector boundary collapse. When vec.IsConst() the code
    returned partitions = [0] and, for IsConstNull(), even cleared
    all of diffs. A const key appearing mid-list therefore collapsed
    all previously found partitions in the same Partition call series.

Fix

In both genericPartition and bytesPartition, treat every "rows are
equal under this key" outcome as a no-op on diffs (the both-NULL
case and the entire const branch). Always rebuild partitions from
the OR-accumulated diffs at the end. diffs[0] = true is set up
front, so the const-vector path now naturally yields [0] via the
final scan rather than via a special branch.

This is the same semantics the non-const non-null path already had
(diffs[i] = diffs[i] || (v != w)).

Tests

  • pkg/partition/partition_test.go::TestPartitionAccumulatesDiffs
    exercises the reuse contract — multiple successive Partition
    calls on the same diffs/partitions slices — across all four
    problematic shapes:
    • fixed-width second key with all NULLs preserves the first key's
      boundaries
    • fixed-width const non-null second key preserves boundaries
    • fixed-width const NULL second key preserves boundaries
    • bytes second key with all NULLs preserves boundaries
    • bytes const second key preserves boundaries
    • sanity check that two non-trivial keys union their boundaries
  • test/distributed/cases/join/fullouterjoin.sql gets the issue's
    exact repro and a NULLS LAST (DESC) secondary-key variant.
  • mo-tester -m run on the test file: 45/45 SUCCESS, 0 FAILED.
  • Unit tests for pkg/sql/colexec/order/...,
    pkg/sql/colexec/window/..., and pkg/partition/... all pass.

PR Type and Checklists

Standard Checklist:

  • Pull request title is following Conventional Commits syntax.
  • Tests added/updated.
  • Documentation is not required (internal correctness fix; user-visible behavior now matches a standard-compliant engine).

Checklist for BUG PR Type:

  • I have added test cases to cover the new code.

…st keys

Fixes matrixorigin#24248.

partition.Partition is called once per sort key in
pkg/sql/colexec/order/order.go::sortAndSend (and per partition spec in
window.go), reusing the same diffs slice across calls. Each call must
*OR* new boundaries onto diffs and never overwrite an existing true.

Two paths violated this:

1. genericPartition / bytesPartition: when both adjacent rows were NULL
   under the current key, the code did diffs[i] = false, erasing a
   boundary set by a prior key. This made multi-key ORDER BY over a
   FULL OUTER JOIN produce rows out of order: with the primary key
   NULL on the right-padded side, the secondary key was never
   sub-sorted within the t1.s=NULL partition (e.g. 14 emitted before
   13).

2. Const branch: vec.IsConst() returned only [0] and IsConstNull()
   even cleared all of diffs. This collapsed previously found
   partitions when a const key appeared mid-list.

Fix: in both functions, treat all 'rows are equal under this key'
outcomes as a no-op on diffs (the both-NULL case and the entire const
branch). Always build partitions from the OR-accumulated diffs at the
end.

Tests: pkg/partition/partition_test.go gets TestPartitionAccumulatesDiffs
covering both fixed-width and bytes paths for: both-NULL preservation,
const non-null preservation, const-NULL preservation, and the
union-of-boundaries case. test/distributed/cases/join/fullouterjoin.sql
gets the issue's repro (the 13/14 ordering case) and a NULLS-LAST
secondary-key variant.

mo-tester: 45/45 SUCCESS.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@qodo-code-review
Copy link
Copy Markdown

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes multi-key ORDER BY / window partition boundary handling when partition.Partition is called repeatedly with a reused diffs slice, ensuring previously-discovered boundaries are preserved for NULL-vs-NULL and const-vector keys (regression #24248).

Changes:

  • Update genericPartition and bytesPartition to never overwrite existing diffs[i]==true with false for “both NULL” and const-vector paths, and always rebuild partitions from accumulated diffs.
  • Add unit coverage to verify the “accumulate diffs across successive Partition calls” contract.
  • Add a distributed regression case for FULL OUTER JOIN + multi-key ORDER BY with NULLs.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
pkg/partition/partition.go Preserves prior boundaries in diffs for NULL/const keys and rebuilds partitions from the accumulated diffs.
pkg/partition/partition_test.go Adds regression/unit tests covering successive Partition calls that reuse the same diffs/partitions slices.
test/distributed/cases/join/fullouterjoin.sql Adds an end-to-end FULL OUTER JOIN + ORDER BY repro for #24248 (including a DESC variant).
test/distributed/cases/join/fullouterjoin.result Updates expected output for the new distributed regression queries.

Comment thread test/distributed/cases/join/fullouterjoin.sql
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/wip kind/bug Something isn't working kind/documentation Improvements or additions to documentation size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants