Preserve $sort document order in $group $push accumulator (fixes #499)

[richardsimmonds]

Summary

Fixes documentdb#499 — $group with $push did not preserve the document order established by a preceding $sort stage, diverging from MongoDB semantics.

Root cause

PostgreSQL gives no ordering guarantee for rows feeding a GroupAggregate: the planner re-sorts on the group key (or uses HashAggregate), which scrambles the carefully $sort-ed input before bson_array_agg sees it. So the accumulated arrays came out in arbitrary order.

Fix

In HandleGroupCore, when handling $push:

• If a compatible sort spec is in effect — either context->sortSpec from an immediately preceding $sort stage, or the suffix sort spec pushed down by the $sortGroup prefix optimization (accumulatorSortSpec) — attach it to the bson_array_agg aggregate as an aggregate-level ORDER BY via the existing SetAccumulatorSortOrder helper.
• This mirrors exactly what is already done for $first/$last ordered accumulators.
• Compatibility is checked with the existing IsSortSpecCompatibleForPushToAccumulatorOperator (rejects $natural and non-numeric directions like $meta).
• Gated to PG16+, consistent with the existing ordered-aggregate index handling (PG15 does not push ordered aggregates to the index and crashes in ProcessOrderByStatements).

AddArrayAggGroupAccumulator gains an optional createdAccumulatorEntry out-param (same pattern as AddSimpleGroupAccumulator) so the caller can reach the Aggref to attach the order-by.

Tests

Extended bson_aggregation_pipeline_tests_push_group with regression tests for issue documentdb#499:

• $sort {_id: 1} → $group + $push: ascending order preserved
• $sort {_id: -1}: descending order preserved
• $sort on a non-_id field
• Same three pipelines re-run after shard_collection to verify sharded behaviour

CI will validate the expected outputs across PG versions.

Notes for reviewers

• The behaviour change only kicks in when context->sortSpec is live (i.e. $sort immediately precedes $group, since any non-order-preserving stage resets sortSpec to EOD) or when the $sortGroup optimization passes a suffix sort — so existing pipelines without a preceding $sort are unaffected.
• Existing EXPLAIN-based tests asserting '$push only → sort NOT pushed' (firstlast/sumavg suites) exercise the $sortGroup optimization path where the Sort node must remain; that decision logic (AnalyzeSortGroupAccumulators) is untouched. The new ORDER-BY attach for $push happens in addition to the retained Sort node, which CI may surface in those EXPLAIN outputs — if so I'll update those expected files.

[richardsimmonds]

Review Summary

Verdict: NEEDS CHANGES — the core fix is correct and well-scoped, but the test story has three holes that will surface as CI failures.

What's good

• Root-cause analysis is right: PostgreSQL gives no input-order guarantee into GroupAggregate, so attaching the sort spec as an aggregate-level ORDER BY is the correct mechanism. It mirrors exactly what SetAccumulatorSortOrder already does for $first/$last (lines 7091–7096), and reuses IsSortSpecCompatibleForPushToAccumulatorOperator for the $natural/$meta guards. No new mechanisms to maintain.
• The createdAccumulatorEntry out-param on AddArrayAggGroupAccumulator follows the established AddSimpleGroupAccumulator pattern, and AddGroupExpression already supported the out-param — the only call-site change is NULL → createdAccumulatorEntry. NULL-safety checks out: the out-param is only requested when pushSortSpec != NULL, and AddGroupExpression unconditionally sets it when non-NULL, so SetAccumulatorSortOrder never sees a NULL TLE.
• DCO: Signed-off-by present on b78c2b9. Single commit, no unrelated changes sneaking in.
• Behaviour is correctly fenced: only fires when context->sortSpec is live (reset by any non-order-preserving stage in the mutate loop) or when the $sortGroup suffix spec is passed down.

Blocking issues

1. PG15 will fail the new regression tests. The ORDER-BY attach is gated to PG_VERSION_NUM >= 160000, but the new tests live in internal/pg_documentdb_distributed, which runs on the PG15 leg of regress_tests.yml (make check, matrix includes 15). On PG15 the fix is inert, so array order remains planner-dependent — the new expected output asserting MongoDB order will fail or flake there. See inline comment for options.

2. Existing EXPLAIN expected outputs not regenerated. The pg_documentdb regress suites EXPLAIN exactly this pipeline shape: firstlast test 36a ("$push only → sort NOT pushed") and sumavg Test 31 ("$push is order-sensitive — sort must be preserved"), across base/_pg16/_pg18 expected variants. Their committed plans show bson_array_agg(...) with no aggorder; on PG16+ this patch adds one, so those files change. The PR body says "if CI surfaces it I'll update those expected files" — that's a known-incomplete state, not a maybe. Run the suites and commit the regenerated outputs in this PR. Also re-run bson_aggregation_stage_inversematch_tests (sql lines 109–111): those pipelines sort and group on the same key, so the new within-group ORDER BY compares all-equal keys and may reshuffle the committed array contents.

3. The new tests never run on PG18. The PG18 CI leg runs make check-no-distributed, which skips internal/pg_documentdb_distributed entirely. Consider duplicating the issue-499 regression cases into the pg_documentdb suite so the fix is exercised on every PG version CI builds.

Non-blocking notes

• No CI checks have run on this PR yet (status is empty), so none of the expected-output files are validated. If the new .out was hand-written rather than generated by make check, expect whitespace/header-padding diffs on top of the issues above.
• Perf: the aggregate-level ORDER BY re-sorts rows within each group (extra bson_orderby eval per row) even though the upstream Sort already ordered them — unavoidable given the planner's re-sort on the group key, and correctness wins, but worth remembering if large-array $push workloads regress.
• if (pushSortSpec != NULL && PG_VERSION_NUM >= 160000) as a runtime condition rather than #if matches the existing usage at the fullscan-clause site (line 7541), so no style complaint.

Final merge call is Patty's; my recommendation is fix items 1–2 (CI-breaking) before this leaves draft, and strongly consider item 3.

Reviewed by Hermes (reviewer profile)

[richardsimmonds]

Blocking (interacts with the new tests): this guard means PG15 keeps the old arbitrary-order behaviour — a defensible call given the ProcessOrderByStatements crash you cite — but the new regression tests in internal/pg_documentdb_distributed assert sorted output and DO run on the PG15 CI leg (make check, matrix.pg_version: 15). On PG15 the array order is whatever the planner's group-key re-sort produces (quicksort, not stable), so those tests will fail or flake there. Options: (a) PG15 alternate expected file, (b) gate the new tests on server version, or (c) un-gate the fix if the PG15 crash is limited to the index-pushdown path rather than plain aggorder. As committed, the matrix cannot pass.

[richardsimmonds]

Logic is right and matches the $first/$last precedent. One consequence to be explicit about: the context->sortSpec branch fires for EVERY $sort → $group+$push pipeline on PG16+, which changes committed EXPLAIN outputs — at minimum firstlast 36a ("$push only → sort NOT pushed") and sumavg Test 31 in pg_documentdb/src/test/regress/expected/ (base, _pg16, _pg18 variants), where bson_array_agg currently has no aggorder. Regenerate and commit those in this PR rather than waiting for CI. Also re-run bson_aggregation_stage_inversematch_tests — its pipelines sort and group on the same key, so the new within-group ORDER BY compares all-equal values and may reshuffle the committed arrays.

[richardsimmonds]

Good test shape — asc/desc/non-_id field plus the sharded re-run mirrors how neighbouring suites cover both topologies, and the trailing $sort {_id:1} keeps row order deterministic. Two coverage gaps: (1) this suite is skipped on the PG18 CI leg (make check-no-distributed excludes internal/), so the fix is never exercised on PG18 — consider duplicating these cases into the pg_documentdb regress suite; (2) on PG15 the fix is compiled out, so these assertions won't hold there (see my comment on the C change).