Skip to content

Fix/typechecker orderby#74

Merged
matiastoro merged 4 commits into
mainfrom
fix/typechecker-orderby
Jul 6, 2026
Merged

Fix/typechecker orderby#74
matiastoro merged 4 commits into
mainfrom
fix/typechecker-orderby

Conversation

@jeanpaulduchens

@jeanpaulduchens jeanpaulduchens commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

fix(typing): endurecer el chequeo de ORDER BY contra valores no ordenables

Rama: fix/typechecker-orderbymain
Alcance: typechecker (compile-time). No toca runtime ni tipado global.

Resumen

Cierra un hueco de type-safety en el chequeo de ORDER BY. Ciertos valores
no ordenables (records constantes, agregados que devuelven una lista, y
variables de repetición) se "lavaban" al tipo desconocido en una etapa
previa y pasaban el chequeo de comparabilidad de ORDER BY. En runtime, ese
ORDER BY se ignoraba en silencio, que es justo el caso que ISO/IEC
39075:2024 §4.4.2 marca como data exception 22G04.

El fix tipa esos tres casos con su tipo real localmente en el chequeo de
ORDER BY
y los rechaza estáticamente, con un error que cita §22.14.

Motivación

está reservado para tipos base (por la relación de precisión, nunca
es consistente con un record ni una lista). Un en una llave de ORDER BY
es sano: siempre es un tipo base, y todo tipo base es ordenable. El problema
es que un valor no base que se lavó a por imprecisión de una etapa
previa se colaba por esa misma puerta. Recuperar su tipo real antes del
chequeo restaura la garantía: un programa bien tipado no falla en runtime por
una llave de orden no comparable.

Rechazar records/listas/nodos/aristas como llave de orden es conforme al
núcleo del estándar
, no una limitación arbitraria: ordenarlos requiere la
feature opcional GA04 (Universal comparison, §4.4.2, §22.14), que froGQL no
implementa. Sin ella, esos valores son equality-comparables pero no
ordering-comparables.

Qué cambia

Tres arreglos locales a check_order_by (src/typing/checker.rs), todos
detrás de un helper order_key_type que recupera el tipo real de la llave:

  1. Agregado de resultado no escalar. Una llave COLLECT_LIST(x) se tipa
    por el tipo del reductor (List), no por , y se rechaza. Los agregados
    escalares (COUNT, SUM, MIN/MAX) siguen aceptándose.
  2. Record constante. Un record literal (posible anidado) se pliega a un
    Value::Record que simple_type_of_value lavaba a ; ahora se tipa como
    Record vía const_order_type, sólo para la llave de orden.
  3. Variable de repetición (Group). Una variable ligada por {n,m} no es
    un tipo base; se tipa como Group y se rechaza como llave.

El blast radius es mínimo: la restricción vive sólo en check_order_by.
GROUP BY, DISTINCT y = usan equality-comparability, que nodos, aristas
y records sí tienen, así que ahí no hay restricción (agrupar por identidad de
nodo o por un record sigue siendo válido). El tipado global de constantes
(Expr::Const) queda intacto: RETURN/WHERE/COALESCE siguen viendo un
record constante como .

Bajar a un campo escalar sigue siendo válido (ORDER BY r.campo), pero no a
un campo que a su vez es un record (ORDER BY r.sub).

Tests

tests/typecheck_gaps_order_by_test.rs (19 casos, +1 ignorado). Fijan cada
caso: rechazar record / record anidado / lista-agregada / grupo como llave;
aceptar RETURN de un record y ORDER BY sobre un campo escalar; y que los
agregados escalares y los aliases sigan resolviéndose.

Commits

  • bebaa9e fix(typing): type ORDER BY aggregate keys by result, reject non-orderable
  • 10dea40 fix(typing): reject constant records as ORDER BY sort keys (local hardening)
  • 9e1bd16 fix(typing): reject repetition-group variables as ORDER BY sort keys
  • bfb35c2 docs(typing): tidy comments, restore simple_type_of_value doc

Pre-commit

cargo fmt --all · cargo clippy --workspace --all-targets -- -D clippy::all
· sweep completo de tests: verde.

Notas para el revisor

  • Ninguna consulta cambia de resultado: sólo se rechazan estáticamente
    consultas que en runtime habrían ignorado el ORDER BY en silencio.
  • No hace falta que records/grupos devuelvan su tipo real en todo el motor;
    la restricción ★ = base es sólo para orden, por diseño.

Jean Paul Duchens Pacheco and others added 3 commits June 25, 2026 15:32
…able

ORDER BY referencing an aggregate column was typed as `Star` ("Aggregate
output typing is a separate gap; pass-through"). That laundered a
non-orderable aggregate result (e.g. `COLLECT_LIST` -> List) past the
§22.14 comparability check: it passed typecheck, then at runtime the
comparator returned "incomparable" -> treated as Equal -> the ORDER BY was
silently ignored (a type-safety hole, since `Star` is reserved for base
types in the gradual design).

Type the aggregate by its result via `check_aggregator` (consistent with
how `Expr::Agg` is already typed: COUNT->Int, SUM->numeric, MIN/MAX->
element type, COLLECT_LIST->List). A List/Record-valued aggregate is now
rejected statically; scalar aggregates keep working. No runtime checks.

Scope: closes the aggregate path only. Other Star-laundering sites (record
constants in `simple_type_of_value`, repetition `Group`, unknown
list/record property values) remain and are a separate decision.

Test: typecheck_rejects_list_aggregate_in_sort_key.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dening)

A constant record (`RECORD { name: 'Alice' }`, all-literal fields) folds
to `Value::Record` and is typed by `simple_type_of_value` as `Star` (the
phase-1 punt). That laundered a non-orderable record past the §22.14
comparability check: it passed typecheck, then at runtime the comparator
returned "incomparable" -> treated as Equal -> ORDER BY silently ignored
(a type-safety hole; `Star` is reserved for base types).

Fix is LOCAL to ORDER BY, mirroring the aggregate fix on this branch:
- new `order_key_type` types a sort-key `Expr::Const` via `const_order_type`,
  which (unlike `simple_type_of_value`) types records — including nested
  records — as `Record`, so `is_orderable_per_iso_22_14` rejects them.
- applied at the three sort-key typing points (SortKey::Expr,
  SortKey::Column, SortKey::ColumnField). The ColumnField path now
  extracts real field types, so `ORDER BY r.scalarField` keeps working
  while `ORDER BY r.recordField` is rejected.
- the global constant typing (`Expr::Const` in `check_expr`) is unchanged,
  so RETURN/WHERE/COALESCE still see a constant record as `Star`. Blast
  radius stays inside ORDER BY. No runtime checks.

Tests: reject const record / nested record field as sort key; accept
RETURN of a const record; accept ORDER BY over a scalar record field.

Scope: closes the record-constant path. The genuinely-unknown property
`Star` case (whether `Star` on a property stays orderable, assuming
base-typed properties) is left as an open design question for review.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Same class of Star-laundering as the record/aggregate cases:
`variable_type_to_simple_type` types a `VariableType::Group` (a `{n,m}`
repetition binding) as `Star` (the projection punt), so `ORDER BY <group
var>` passed the §22.14 comparability check and then silently no-op'd at
runtime (the group projects to Null -> all "Equal" -> no reorder).

Extend the local `order_key_type` (ORDER BY only) to type a group
variable as `Group` so the check rejects it. The global
`variable_type_to_simple_type` punt is unchanged, so projection of a
group keeps its current behaviour. Consistent with "Star is a base type
only": a group is not a base type and must not be laundered to Star at a
comparability site.

Test: `ORDER BY` over a `{1,2}` repetition group is rejected.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jeanpaulduchens jeanpaulduchens self-assigned this Jul 5, 2026
@jeanpaulduchens jeanpaulduchens added the enhancement New feature or request label Jul 5, 2026
…f_value doc

Consolidate the record/group laundering rationale into order_key_type's
doc comment and drop the per-arm repeats. Re-attach the doc comment that
const_order_type's insertion had split off simple_type_of_value, and
trim the test comments that re-explained the same mechanism.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@matiastoro matiastoro merged commit 21e37af into main Jul 6, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants