Skip to content

Reconcile MERGE ORDER BY with its synthesized grouping instead of leaking a raw engine binder error #196

Description

@conradbzura

Description

MERGE aggregates rows into one row per merged region. _transform_for_merge (src/giql/expanders/merge.py) copies the user's ORDER BY verbatim onto the final aggregation query without reconciling it against the synthesized per-merge grouping. An ORDER BY over a column that is neither a grouping key (chrom, and strand when stranded) nor an aggregate is not well-formed under that grouping, so the target engine rejects it with an opaque, engine-specific error:

SELECT chrom, MERGE(interval) FROM peaks ORDER BY score DESC
-> duckdb.BinderException: column "score" must appear in the GROUP BY clause or must be part of an aggregate function

SELECT chrom, MERGE(interval) FROM peaks ORDER BY nonexistent DESC
-> duckdb.BinderException: Referenced column "nonexistent" not found

This is an asymmetry introduced by #192: GROUP BY score now raises a clear giql ValueError ("MERGE cannot honor GROUP BY score: ..."), while the analogous ORDER BY score still surfaces a raw target-engine binder error, so a user cannot tell from the giql layer why the query failed.

This is a diagnostics/robustness gap, not a correctness bug: the query fails loudly and never returns wrong rows. It was found during the holistic Round-3 review of PR #193 (reviewer finding F3) and deliberately left out of that PR's scope because it fails loudly rather than silently.

Expected behavior

An ORDER BY term that references, outside an aggregate, a column that is not one of MERGE's grouping keys (nor a projected merged-output column) should raise a clear giql ValueError naming the offending column and pointing at the supported shapes — the same clean diagnostic the projection and GROUP BY reconciliation already produce — instead of passing through to a raw engine binder error.

Already-correct behavior that must be preserved: ordering over a merged bound (ORDER BY "end" DESC over MAX("end") AS end), ordering over a grouping key, and the deterministic default ORDER BY "chrom", "start" when no user ORDER BY is present (#181).

Root cause

_transform_for_merge honors the user's ORDER BY by copying it verbatim onto the outer aggregation query (final_query.set("order", query.args["order"].copy())) with no reconciliation, unlike the projection items and GROUP BY, which are validated against the per-merge grouping (#192). The fix is to reconcile ORDER BY the same way — every column an ORDER BY term references outside an aggregate must be a grouping key (or resolve to a projected merged-output column), else raise — reusing the existing grouping_keys_folded and ungrouped/window-aware column checks in _transform_for_merge.

Related: #192 (the reconciliation this extends), #181 (MERGE honors the user's ORDER BY over merged output).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions