Skip to content

feat(lapis): add sequence position fields syntax to aggregated endpoint - #1768

Open
fhennig wants to merge 26 commits into
mainfrom
feat/sequence-position-fields
Open

feat(lapis): add sequence position fields syntax to aggregated endpoint#1768
fhennig wants to merge 26 commits into
mainfrom
feat/sequence-position-fields

Conversation

@fhennig

@fhennig fhennig commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds SequenceName[position] syntax (e.g. S[501]) to the fields parameter of /aggregated, allowing users to request specific sequence positions as group-by columns
  • Shorthand [N] is supported for single-segmented genomes and resolves to the single nucleotide sequence
  • Sequence names are validated case-insensitively against both genes and nucleotide segments from the reference genome
  • /details, phylo tree, and MRCA endpoints reject position fields at the type level (they only ever construct a PlainField), with a clear error message
  • GET /aggregated's fields query parameter now documents the position-field syntax too, matching what the POST body schema already described
  • Generates a .map({"S[501]":="S".at(501)}) SaneQL step before .groupBy(...) when position fields are present — the user-facing name is used directly as the SaneQL alias, so no post-processing is needed
  • Response column keys mirror the input exactly: S[501] in → S[501] out, [501] in → [501] out
  • Internal cleanup: split the field/orderBy converters and cleaners out of Field.kt/OrderByField.kt into a dedicated request.converter package, renamed the sealed field hierarchy for clarity (RequestFieldField, FieldPlainField), and renamed SequenceFiltersRequestWithFieldsAggregatedFiltersRequest to match its /aggregated-only sibling naming (DetailsFiltersRequest, PhyloTreeSequenceFiltersRequest, MRCASequenceFiltersRequest)

Example

POST /sample/aggregated
{"fields": ["country", "S[501]"]}

Generates:

default.filter(true).map({"S[501]":="S".at(501)}).groupBy({"count":=count()}, {"country", "S[501]"})

Test plan

  • Unit tests for SequenceName[position] parsing in AggregatedFiltersRequestTest
  • Error cases: unknown sequence name, position ≤ 0, shorthand on multi-segmented genome
  • SaneQL generation tests in SiloQueryToSaneQlTest for position-only and mixed fields
  • e2e coverage for a gene (amino acid) position, a plain nucleotide position, and the single-segment shorthand syntax
  • Full test suite passes

🤖 Generated with Claude Code

Allow users to request specific sequence positions as group-by fields
using `Name[N]` syntax (e.g. `S[501]`). For genomes with a sequence
named `main`, the shorthand `[N]` is also accepted. Position fields
generate a `.map(...)` step in SaneQL before `.groupBy(...)`, and
appear in the response under the alias `Name_N` (e.g. `S_501`).

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

vercel Bot commented Jul 14, 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 2:36pm

Request Review

Echo S[501] back as the response column key instead of the internal
SaneQL alias S_501, so input and output are symmetric for callers.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ack in response

Shorthand [N] now validates via isSingleSegmented() (consistent with
how the rest of the codebase handles single-segment-only features)
rather than checking for a hardcoded 'main' name.
SequencePositionField gains isSingleSegment flag so the response key
mirrors the input: [501] in → [501] out, S[501] in → S[501] out.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…field aliasing

SaneQlAssignment now quotes its name (consistent with SaneQlIdentifier,
safer against injection). Position fields use their user-facing name
(e.g. S[501]) directly as the SaneQL alias, so SILO returns the column
under that name already — no post-processing remapping needed in the
controller.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <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 requesting specific sequence positions (e.g. S[501], and [501] for single-segment genomes) as group-by columns in the /aggregated endpoint by parsing them from the fields request parameter and generating the appropriate SaneQL .map(...).groupBy(...) pipeline.

Changes:

  • Introduces RequestField with a new SequencePositionField type and updates request parsing to recognize Name[N] / [N] syntax.
  • Extends aggregated SaneQL generation to add a .map(...) step that computes requested positions and includes them in .groupBy(...).
  • Updates SaneQL AST rendering/tests to quote assignment keys (e.g. {"count":=count()}) and adds unit coverage for new parsing/generation.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
lapis/src/main/kotlin/org/genspectrum/lapis/request/Field.kt Adds RequestField + SequencePositionField, and extends field conversion to parse/validate position syntax against the reference genome schema.
lapis/src/main/kotlin/org/genspectrum/lapis/request/SequenceFiltersRequestWithFields.kt Updates request model to store fields as List<RequestField> and keeps deserialization via the field converter.
lapis/src/main/kotlin/org/genspectrum/lapis/silo/SiloQuery.kt Adds sequencePositionFields to aggregated actions and emits .map(...) before .groupBy(...) when present.
lapis/src/main/kotlin/org/genspectrum/lapis/silo/SaneQlAst.kt Changes assignment rendering to quote/escape keys (e.g. "name":=value) to align with SaneQL expectations and harden output.
lapis/src/main/kotlin/org/genspectrum/lapis/model/SiloQueryModel.kt Splits RequestField values into metadata fields vs. sequence-position fields for aggregated query construction.
lapis/src/main/kotlin/org/genspectrum/lapis/controller/LapisController.kt Ensures response headers/field lists use outputColumnName (aggregated) and filters non-metadata fields out of details fields handling.
lapis/src/main/kotlin/org/genspectrum/lapis/controller/ControllerDescriptions.kt Updates aggregated endpoint parameter description to document the new position-field syntax.
lapis/src/main/kotlin/org/genspectrum/lapis/request/PhyloTreeSequenceFiltersRequest.kt Adjusts phylo-tree parsing to work with the generalized FieldConverter<RequestField> while enforcing a Field result.
lapis/src/test/kotlin/org/genspectrum/lapis/request/SequenceFiltersRequestWithFieldsTest.kt Adds unit tests for parsing valid/invalid Name[N] / [N] field forms.
lapis/src/test/kotlin/org/genspectrum/lapis/silo/SiloQueryToSaneQlTest.kt Updates expected SaneQL strings and adds coverage for aggregated SaneQL with sequence position fields.
lapis/src/test/kotlin/org/genspectrum/lapis/silo/SaneQlAstTest.kt Updates AST rendering assertions to match quoted assignment output.

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

Comment thread lapis/src/main/kotlin/org/genspectrum/lapis/controller/ControllerDescriptions.kt Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@fhennig

fhennig commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Ok, something weird with the test? But still reviewable I think. I'll look into it.

…ns.kt

The AGGREGATED_GROUP_BY_FIELDS_DESCRIPTION const was missing its closing
triple-quote, which caused the compiler to swallow the following
declaration as part of the string and fail with unresolved reference
and syntax errors.

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

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

Comment thread lapis/src/main/kotlin/org/genspectrum/lapis/request/Field.kt Outdated
Comment thread lapis/src/main/kotlin/org/genspectrum/lapis/controller/LapisController.kt Outdated
Previously SequencePositionField entries were silently dropped, causing
/details to return a 200 with all metadata fields instead of erroring.
Now it returns a 400 with a clear message, per Copilot review feedback.

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

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

Comment thread lapis/src/main/kotlin/org/genspectrum/lapis/silo/SaneQlAst.kt
Comment thread lapis/src/main/kotlin/org/genspectrum/lapis/silo/SiloQuery.kt
Comment thread lapis/src/main/kotlin/org/genspectrum/lapis/request/DetailsFiltersRequest.kt Outdated
Comment thread lapis/src/main/kotlin/org/genspectrum/lapis/request/Field.kt Outdated
Comment thread lapis/src/main/kotlin/org/genspectrum/lapis/openApi/OpenApiDocs.kt
Comment thread lapis-e2e/test/aggregatedQueries/sequencePositionField.json
Keeps the sealed type named Field, matching how it reads at call sites
(fields: List<Field>), and gives the plain-name variant its own name
now that SequencePositionField is also a Field.

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

Field.kt and OrderByField.kt mixed request data types with the
converter/cleaner classes that produce them, making the files harder
to scan. Converters and cleaners (FieldConverter, MetadataFieldConverter,
AggregatedFieldConverter, PlainFieldConverter, SequencePositionFieldConverter,
CaseInsensitiveFieldsCleaner, OrderByFieldsCleaner, OrderByFieldConverter)
now live in a dedicated request.converter package; request data types
(Field, PlainField, SequencePositionField, OrderBySpec, OrderByField, Order)
and Jackson deserializers stay in request. Pure move, no behavior change.

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

userFacingName duplicated outputColumnName exactly; collapse into the
single outputColumnName property that the sealed Field interface already
defines.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The `fields` query param's OpenAPI description was still the static
@FieldsToAggregateBy text, unlike the POST body schema which already
used the dynamic aggregatedFieldsDescription(). Since that depends on
the reference genome it can't live on the annotation, so patch it in
via a small OpenApiCustomizer bean, following the existing
securityCustomizer/headerCustomizer pattern in LapisSpringConfig.

Also drops the llms.txt test for sequence position field docs.

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

The only existing sequencePositionField test covers a gene (amino acid)
position. Add a case for a plain nucleotide position on the main
sequence, plus one for the single-segment shorthand syntax, since
neither code path had e2e coverage.

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

This type is exclusively used by the /aggregated endpoint, unlike its
sibling SequenceFiltersRequestWithGenes which is shared across several
amino-acid-sequence endpoints. Rename it to match the naming pattern of
its other /aggregated-specific siblings (DetailsFiltersRequest,
PhyloTreeSequenceFiltersRequest, MRCASequenceFiltersRequest).

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

The previous commit only moved the file paths; a botched git add left
the actual class/reference renames uncommitted. This completes it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… syntax and dedupe GET fields

Sequence position field parsing (S[501]) is tried before metadata field
lookup, so a metadata field literally named like Name[digits] would be
unreachable or misresolved. Ban '[' and ']' in metadata field names to
close that hole. Also apply the same .distinct() dedup to GET field-list
endpoints that the POST body path already had, so duplicate fields
(e.g. via repeated query params) don't produce duplicate SaneQL columns.

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

Copilot reviewed 41 out of 41 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

lapis/src/main/kotlin/org/genspectrum/lapis/silo/SaneQlAst.kt:74

  • The SaneQlList KDoc above still uses the pre-change assignment example ({count:=count()}), but SaneQlAssignment now renders as {"count":=count()}. Update the example to match the new rendered syntax to avoid misleading documentation.
    override fun render() = "{" + items.joinToString(", ") { it.render() } + "}"
}

/** A `"name":=value` assignment, used inside [SaneQlList]s that act as records, e.g. `{"count":=count()}`. */
data class SaneQlAssignment(

lapis/src/main/kotlin/org/genspectrum/lapis/request/converter/SequencePositionFieldConverter.kt:8

  • SEQUENCE_POSITION_REGEX is more restrictive than other sequence-name parsers (e.g., mutations allow - via [a-zA-Z0-9_-]+). This prevents using position fields for valid segment/gene names that contain hyphens (or start with digits), even though the reference genome schema does not restrict those names.
private val SEQUENCE_POSITION_REGEX = Regex("""^([A-Za-z][A-Za-z0-9_]*)?\[(\d+)]$""")

lapis/src/main/kotlin/org/genspectrum/lapis/request/converter/PlainFieldConverter.kt:21

  • PlainFieldConverter aims to reject sequence position fields with a dedicated error, but tryConvert() can throw (e.g., unknown sequence or shorthand on multi-segment genomes), which bypasses this message and surfaces a different one. Catch those cases and consistently rethrow the “not supported here” error for any input that matches position-field syntax.
    override fun convert(source: String): PlainField {
        sequencePositionFieldConverter.tryConvert(source)?.let {
            throw BadRequestException(
                "Sequence position fields are not supported here: ${it.outputColumnName}",
            )
        }
        return metadataFieldConverter.convert(source)
    }

Comment on lines +14 to +17
if (it.name.contains('[') || it.name.contains(']')) {
throw IllegalArgumentException(
"Metadata field name '${it.name}' contains a reserved character, '[' or ']'",
)

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.

👍

Comment on lines +122 to +124
* The GET `fields` parameter is documented via the static `@FieldsToAggregateBy` annotation, which can't hold
* the dynamic [aggregatedFieldsDescription] (it depends on the reference genome). Overwrite it here after the
* fact, matching what the POST body's `AGGREGATED_REQUEST_SCHEMA` already documents.

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.

Then let's maybe change the original description in the annotation in the controller? Either remove it or make it say "I'm just a placeholder that will be overwritten later"?

Comment on lines +128 to +133
OpenApiCustomizer { openApi ->
openApi.paths["/sample$AGGREGATED_ROUTE"]
?.get
?.parameters
?.find { it.name == FIELDS_PROPERTY }
?.description = aggregatedFieldsDescription(referenceGenomeSchema)

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.

I just noticed that we have a SampleEndpointsGetParameterCustomizer, maybe we should put it there?

Also the SwaggerUI of the multi segmented local instance doesn't load. Generating the API docs fails:

2026-07-28 08:53:57,290 WARN [http-nio-8094-exec-4] [4bcc64a3-e491-4a89-8278-d33272b0fb65] org.genspectrum.lapis.controller.ExceptionHandler: Caught unexpected exception: getContent(...) must not be null
java.lang.NullPointerException: getContent(...) must not be null
        at org.genspectrum.lapis.openApi.OperationsSorter.customize$lambda$0(OperationsSorter.kt:18)
        at org.genspectrum.lapis.openApi.OperationsSorter.customize$lambda$1(OperationsSorter.kt:16)
        at java.base/java.util.LinkedHashMap.forEach(LinkedHashMap.java:986)
        at org.genspectrum.lapis.openApi.OperationsSorter.customize(OperationsSorter.kt:16)

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 aggregation by sequence positions

3 participants