Skip to content

Let a query order by a value under a repeated level - #979

Open
skearnes wants to merge 1 commit into
mainfrom
agent/reduce-over-repeated-level
Open

Let a query order by a value under a repeated level#979
skearnes wants to merge 1 commit into
mainfrom
agent/reduce-over-repeated-level

Conversation

@skearnes

@skearnes skearnes commented Aug 18, 2026

Copy link
Copy Markdown
Member

Summary

Order.key and Measure.path both required a scalar path, so a value under a repeated level could not be ordered or aggregated by:

outcomes.products.measurements.percentage.value: max needs a scalar column, not a repeated level

That makes "the ten highest-yielding reactions" unwritable. It surfaced while measuring natural-language translation: both Haiku 4.5 and Opus 5 reached for it independently, and it was Opus's only failure across ten questions — unrepairable, because nothing they could have written would have worked. See ord-logbook#45, finding 7.

Changes

  • Reduction{"reduce": "min"|"max"|"avg"|"sum"|"count", "path": ...} — names how to reduce a repeated path to one value per reaction. Order.key and Measure.path now accept one.
  • resolve() already returns a list expression for a repeated path, so this compiles to a single DuckDB list aggregate around what the resolver produces. count filters nulls rather than using len, which would count them.
  • Refused two ways: over a scalar path (which needs no reduction, and accepting it would give the same query two spellings), and as an aggregated query's ordering key (after grouping there is no reaction left to reduce over — the reduction belongs inside a measure).
  • README: the grammar block, the rule, and a worked-example row.

Testing

uv run pytest -n auto — 1210 passed. Ten new tests: each reducer's expression, both refusals, a measure reducing, agreement with what resolve() produces, and one that executes the SQL.

Against the full corpus (2,428,291 reactions) the previously-unwritable query now runs:

10 rows in 2.82s

And it is right, not merely runnable — the compiled SQL returns the same ten reactions in the same order as a hand-written reference query:

compiled : ord-50b993b6..., ord-aaba6e4b..., ord-09fe18d4..., ord-4951e7a3..., ord-c889cb5c...
reference: ord-50b993b6..., ord-aaba6e4b..., ord-09fe18d4..., ord-4951e7a3..., ord-c889cb5c...
same set : True

Notes

Worth knowing separately from this change: the top yields in ORD are junk — the highest is 9.02e+19 percent. The query is correct; the corpus has percentage values that are not percentages. That is a data-quality question for another day, but anything that ranks by yield will surface it.

🤖 Generated with Claude Code

Greptile Summary

The PR adds per-reaction reductions over repeated paths for ordering and aggregate measures, along with query documentation and tests.

  • Introduces Reduction with min, max, average, sum, and non-null count operations.
  • Allows reductions as non-aggregate ordering keys and measure arguments while rejecting scalar paths and post-grouping reductions.
  • Documents the new grammar and highest-yield query pattern.

Confidence Score: 4/5

The incompatible reducer and leaf-type combinations should be rejected before this PR is merged because currently valid query payloads can produce DuckDB type errors.

The reduction compiler verifies repeatedness but ignores the resolved leaf type, allowing arithmetic list reducers to be emitted for concrete repeated VARCHAR paths.

Files Needing Attention: ord_schema/search/query.py

Important Files Changed

Filename Overview
ord_schema/search/query.py Adds reduction models and SQL compilation, but permits arithmetic reducers over incompatible nonnumeric repeated leaves.
ord_schema/search/query_test.py Covers reducer SQL generation, scalar and aggregate refusals, and an empty-table execution smoke test, but not incompatible leaf types.
ord_schema/search/README.md Updates the grammar, reduction constraints, and worked examples consistently with the implementation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Q[Reduction in query] --> R[Resolve repeated path]
  R --> L[Flat list expression per reaction]
  L --> D[List reducer]
  D --> S[Scalar value per reaction]
  S --> O[ORDER BY]
  S --> A[Aggregate measure]
Loading

Reviews (1): Last reviewed commit: "Let a query order by a value under a rep..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

An ordering key and an aggregate's argument both had to be scalar, which
left "the ten highest-yielding reactions" unwritable: a yield lives
under outcomes, products, and measurements, so the path resolves to a
list rather than a number. Both a cheap model and an expensive one
reached for it independently while translating questions, and neither
could have succeeded.

A Reduction names how to reduce that list to the one value the reaction
is judged by, and resolve() already returns a list expression for a
repeated path, so it compiles to one DuckDB list aggregate around what
the resolver produces. It is refused over a scalar path, which needs no
reduction, and inside an aggregated query's order_by, where there is no
reaction left to reduce over.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f"{reduction.path}: {reduction.reduce} reduces a repeated level, and this "
f"path is already scalar; order by the path itself"
)
return _REDUCERS[reduction.reduce].format(expression=resolved.expression)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Reducer ignores resolved leaf type

When an arithmetic reduction such as sum or avg targets a repeated nonnumeric path like outcomes.products.measurements.string_value, _reduced() emits a DuckDB list aggregate without validating the resolved leaf type, causing the query to fail with a DuckDB type error instead of a compile-time QueryError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant