Skip to content

feat(lapis): support scalar functions in aggregation fields via dot notation - #1780

Draft
fhennig wants to merge 7 commits into
mainfrom
feature/scalar-functions-in-aggregation-fields
Draft

feat(lapis): support scalar functions in aggregation fields via dot notation#1780
fhennig wants to merge 7 commits into
mainfrom
feature/scalar-functions-in-aggregation-fields

Conversation

@fhennig

@fhennig fhennig commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

WIP

resolves #1767

Summary

Users can now specify computed fields like date.isoWeek in the fields parameter of the /aggregated endpoint. LAPIS validates the function against a whitelist (currently isoWeek for date fields), generates a map() step before groupBy() in the SaneQL query, and renames the internal alias columns back to the user-facing field.function names in the response.

PR Checklist

  • All necessary documentation has been adapted.
  • All necessary changes are explained in the llms.txt.
  • The implemented feature is covered by an appropriate test.

…otation

Users can now specify computed fields like `date.isoWeek` in the `fields`
parameter of the `/aggregated` endpoint. LAPIS validates the function against
a whitelist (currently `isoWeek` for date fields), generates a `map()` step
before `groupBy()` in the SaneQL query, and renames the internal alias columns
back to the user-facing `field.function` names in the response.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 23, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
lapis Ready Ready Preview, Comment Jul 27, 2026 9:19am

Request Review

fhennig and others added 3 commits July 27, 2026 10:09
Add a doc comment to ScalarFunction explaining new functions must be
whitelisted there, and describe the <field>.<function> syntax in the
OpenAPI description for the aggregated endpoint's fields parameter.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cover stratifying and ordering by a computed field (date.isoWeek),
bad requests for unknown functions and wrong field types, and that
computed fields are rejected on /details. Verified against a real
SILO instance.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add a concepts page explaining the <field>.<function> dot-notation
syntax for the fields parameter (e.g. date.isoWeek), and link it from
the references introduction.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@fhennig

fhennig commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Code review (high effort)

Reviewed git diff main...HEAD for this branch. One confirmed correctness bug, plus a few lower-severity cleanup findings.

🐞 Correctness bug (CONFIRMED)

orderBy references to computed fields are not case-normalized the way fields entries arelapis/src/main/kotlin/org/genspectrum/lapis/model/SiloQueryModel.kt:74

OrderByFieldsCleaner.clean (request/OrderByField.kt:148) only delegates to CaseInsensitiveFieldsCleaner.clean, which has no awareness of the <field>.<function> dot syntax and no dot-splitting logic. For a dotted orderBy value it returns null and falls back (?: fieldName) to the raw, un-normalized string the client sent — unlike fields, which always goes through CaseInsensitiveFieldConverter.convert and produces a canonical "$sourceField.${function.saneQlMethodName}" string.

rewriteOrderByForComputedFields's userFacingToAlias[field.field] lookup is an exact, case-sensitive match against that canonical form.

Repro: fields=["date.isoWeek"], orderBy=[{field:"DATE.ISOWEEK"}] (or any casing variant differing from the canonical form). The alias lookup misses, the substitution silently doesn't happen, and the raw string "DATE.ISOWEEK" is emitted verbatim via id() as a literal SaneQL column identifier — a column that doesn't exist in the post-groupBy result (only date/__scalar_isoWeek_date exist), causing an unexpected SILO-side error instead of the case-insensitive behavior the rest of the API guarantees for plain fields.

None of the added tests exercise this path — SiloQueryModelTest constructs the orderBy field directly from isoWeekField.fieldName in memory, bypassing the real cleaner/converter entirely, so the mismatch is untested.

🧹 Cleanup findings (lower severity)

  1. Repeated scans of the same listSiloQueryModel.kt:43-55. getAggregated calls sequenceFilters.fields.filterIsInstance<Field.Plain>() / filterIsInstance<Field.Computed>() four separate times over the same list instead of partitioning once. Minor per-request cost, and any future change to how computed fields are derived needs updating in three call sites instead of one.

  2. Duplicated lowercase lookup maprequest/Field.kt:38-39. fieldTypesByLowercaseName rebuilds the same metadata.associateBy(name.lowercase()) that CaseInsensitiveFieldsCleaner.fieldsMap already builds from the identical source list. Currently safe since both derive from the same source, but the non-null assertion at Field.kt:69 relies on that invariant holding — if the two maps ever drift, a BadRequestException turns into an unhandled NPE.

  3. Computed-field support is bolted onto AggregatedAction onlysilo/SiloQuery.kt:229 onward. The computedFields/map-step/alias/rename/orderBy-rewrite logic is wired specifically into AggregatedAction and SiloQueryModel, with other endpoints rejecting computed fields via an ad hoc throw in the controller (LapisController.kt:1494-1499) rather than a shared capability. If scalar functions are later extended to another endpoint, this whole mechanism will likely be copy-pasted rather than reused.


Note: I also checked whether the dot-notation parsing regresses metadata fields whose names literally contain a . (since convert() treats any dotted string as computed-field syntax) — this is not an issue, DatabaseConfigValidator already rejects . in field names at config-load time, predating this branch.

🤖 Generated with Claude Code

orderBy previously used a plain-field-only cleaner, so a computed field
(e.g. "date.isoWeek") with different casing than in `fields` would not
be recognized as the same field, silently breaking the alias rewrite
and sending an invalid column reference to SILO.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
computedFields was recomputed via filterIsInstance three separate
times over the same list; compute it once and reuse it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds support for scalar/computed fields (e.g. date.isoWeek) in the fields parameter for the /aggregated endpoint by introducing a typed Field model, validating scalar functions against a whitelist, generating a pre-groupBy() map() step in SaneQL, and translating internal alias columns back to user-facing keys in the response.

Changes:

  • Introduces Field.Plain vs Field.Computed plus ScalarFunction whitelist and validation (type-checked).
  • Extends aggregated SaneQL generation with a map() step for computed fields and adjusts ordering/response key rewriting.
  • Adds unit + e2e tests and documentation for computed fields; blocks computed fields in /details.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
lapis/src/main/kotlin/org/genspectrum/lapis/request/Field.kt Introduces Field sealed type and parsing for <field>.<function> computed syntax with type validation.
lapis/src/main/kotlin/org/genspectrum/lapis/request/ScalarFunction.kt Adds scalar function whitelist (currently isoWeek for DATE).
lapis/src/main/kotlin/org/genspectrum/lapis/request/OrderByField.kt Canonicalizes orderBy fields using the same converter as fields (supports computed fields casing).
lapis/src/main/kotlin/org/genspectrum/lapis/silo/SiloQuery.kt Adds computed-field map() step before groupBy() and groups by computed aliases.
lapis/src/main/kotlin/org/genspectrum/lapis/model/SiloQueryModel.kt Splits plain vs computed fields, rewrites computed orderBy to aliases, and renames aliases back in responses.
lapis/src/main/kotlin/org/genspectrum/lapis/model/mutationsOverTime/QueriesOverTimeModel.kt Updates aggregated call site to named parameters after signature expansion.
lapis/src/main/kotlin/org/genspectrum/lapis/controller/LapisController.kt Rejects computed fields for /details requests with a 400.
lapis/src/main/kotlin/org/genspectrum/lapis/controller/ControllerDescriptions.kt Documents computed-field dot notation in aggregated fields OpenAPI description.
lapis/src/test/kotlin/org/genspectrum/lapis/silo/SiloQueryToSaneQlTest.kt Adds SaneQL rendering test for computed fields (map + groupBy alias).
lapis/src/test/kotlin/org/genspectrum/lapis/silo/SiloQueryTest.kt Updates aggregated action tests to named parameters.
lapis/src/test/kotlin/org/genspectrum/lapis/request/SequenceFiltersRequestWithFieldsTest.kt Updates tests to use Field.Plain.
lapis/src/test/kotlin/org/genspectrum/lapis/request/ScalarFunctionFieldTest.kt New tests for computed-field parsing/validation.
lapis/src/test/kotlin/org/genspectrum/lapis/request/OrderByFieldConverterTest.kt Adds coverage for computed fields in orderBy conversion/canonicalization.
lapis/src/test/kotlin/org/genspectrum/lapis/model/SiloQueryModelTest.kt Adds tests for map-step emission, alias renaming, and orderBy alias rewriting.
lapis/src/test/kotlin/org/genspectrum/lapis/model/mutationsOverTime/Helpers.kt Updates aggregated action helper call site to named parameters.
lapis/src/test/kotlin/org/genspectrum/lapis/controller/LapisControllerCommonFieldsTest.kt Updates controller tests to use Field.Plain.
lapis/src/test/kotlin/org/genspectrum/lapis/controller/Helpers.kt Updates request helper to construct Field.Plain.
lapis-e2e/test/details.spec.ts Adds e2e assertion that computed fields are rejected on /details.
lapis-e2e/test/aggregated.spec.ts Adds e2e coverage for computed fields in /aggregated including ordering and error cases.
lapis-docs/src/content/docs/references/introduction.mdx Links to computed fields concept doc.
lapis-docs/src/content/docs/concepts/computed-fields.mdx New documentation page describing computed fields and available functions.
lapis-docs/astro.config.mjs Adds “Computed fields” to docs navigation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +67 to +70
fun `isoWeek on non-date field throws BadRequestException`() {
val ex = assertThrows<BadRequestException> { underTest.convert("country.isoWeek") }
assert(ex.message.orEmpty().contains("STRING")) { "Expected error to mention type, got: ${ex.message}" }
}
Comment on lines +72 to +74
A field can also be a computed field of the form \"<field>.<function>\" (e.g. \"date.isoWeek\"), which applies a
scalar function to the field and stratifies by the result. The response uses the full \"<field>.<function>\" string
as the field name. Currently, the only supported function is \"isoWeek\", which is only valid for date fields."""
…eries

SILO accepts a quoted identifier containing a literal "." as a column
name (verified against a live SILO instance), so the computed field's
canonical name (e.g. "date.isoWeek") can be used directly as the SaneQL
map/groupBy/orderBy column instead of routing through an internal
__scalar_<function>_<field> alias. This removes the response-column
rename and orderBy-rewrite steps entirely, along with the class of bugs
that comes from keeping two names in sync.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

Add scalar function to aggregation fields

2 participants