Repository navigation
Fixed Cartesian product of optional properties in generated queries - #183
Merged
karelklima merged 1 commit intoOct 13, 2026
Merged
Conversation
- Give each optional property that is fetched but not searched its own branch in a UNION, instead of an OPTIONAL of its own, so the number of solutions is the sum of the number of values per property rather than their product. - Keep OPTIONAL with a FILTER for optional properties that carry a search condition. - Emit the branches after the other patterns of the same level; variable names stay the same.
There was a problem hiding this comment.
🟢 Approval recommended
The query change preserves search behavior and is covered by focused regression tests.
0 open findings
What changed in this PR
Fixes Cartesian-product amplification when fetching independent optional properties.
Changes:
- Emits unsearched optional properties as separate
UNIONbranches. - Adds end-to-end coverage for nested optionals and solution counts.
| File | Description |
|---|---|
library/lens/query_builder.ts |
Generates additive optional-property branches. |
tests/e2e/optional.test.ts |
Tests entity decoding and duplicate-quad reduction. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
karelklima
approved these changes
Oct 13, 2026
karelklima
left a comment
karelklima
left a comment
Owner
There was a problem hiding this comment.
Looks good to me, thanks for the PR!
karelklima
pushed a commit
that referenced
this pull request
Oct 13, 2026
Follow-up to #183. That PR starts the `UNION` of optional properties with an empty group, which keeps the solution for an entity that has none of them. The empty group also adds a solution for every entity that does have them, at each level of a nested schema. A store that does not deduplicate `CONSTRUCT` output, such as QLever, sends the triples of each of those solutions again. With this change the `UNION` sits in an `OPTIONAL` instead: ```sparql ?iri x:requiredString ?iri_0 . OPTIONAL { { ?iri x:optionalArray ?iri_1 . } UNION { ?iri x:otherOptionalArray ?iri_2 . } } ``` An entity with 3 values for one optional array and 3 for another now comes back as 3 + 3 = 6 solutions, where 2.10.0 gives 1 + 3 + 3 = 7. An entity with none still comes back as 1. A level with one optional property gets the same shape, which amounts to the plain `OPTIONAL` it had before #183. ## Measurements I ran `findByIri()` against the QLever endpoint of the [Dataset Knowledge Graph](https://github.com/netwerk-digitaal-erfgoed/dataset-knowledge-graph), on its two largest VoID descriptions, with two schemas: - class partitions: three nested levels, with one, one and three optional arrays; - summary: one level with six optional values and three optional arrays. Response size: | | 2.7.2 | 2.10.0 | this change | | --- | --- | --- | --- | | class partitions, `som-persons-in-music` | 9968 KB | 14792 KB | 9984 KB | | class partitions, `iisg-kg` | 8849 KB | 12488 KB | 9460 KB | | summary, `som-persons-in-music` | 8736 KB | 389 KB | 388 KB | | summary, `iisg-kg` | 8280 KB | 324 KB | 324 KB | The last column is the 2.10.0 npm build with this change applied to it by hand, not a build of this branch. I compared the decoded entities whole, ignoring array order: they are identical in all three columns. On the nested schema 2.10.0 was 48% and 41% larger than 2.7.2. `iisg-kg` stays 7% above 2.7.2: where two optional properties on one level each have a single value, their sum (2 solutions) is higher than their product (1). ## An alternative that does not work I first made only the first branch optional: `{ OPTIONAL { … } } UNION { … }`. That group is evaluated without the bindings of `?iri`, so an entity with none of the optional properties is lost. On `iisg-kg` that dropped 1085 of the 3148 property partitions: the ones without a nested partition. ## Tests The quad count in “Should not multiply the values of optional properties” goes from 34 to 28.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #175.
find()andfindByIri()wrap every@optionalproperty in anOPTIONALof its own, and consecutiveOPTIONALs multiply. An entity with 3 values for one optional array and 3 for another comes back as 9 solutions.With this change, each optional property that is fetched but not searched gets its own branch in a
UNION, so it comes back as 1 + 3 + 3 = 7:@schemagets the same treatment inside its branch.OPTIONALplusFILTERfrom fix(lens): apply search filters on @optional properties (#170) #176: that filter has to see the value in the same solution as the rest of the entity.Required properties are untouched, so a required
@arrayor@multilangproperty still multiplies against the others. I left that alone because those patterns also decide whether an entity matches.Measurements
I ran this against the dataset catalog of the Network of Terms: 57 datasets in an in-memory store, queried with Comunica 5.4 through a schema with 9 optional properties over three levels.
find()The timings are the middle of five runs on my laptop. The decoded entities are identical.
I have not measured QLever, the store from the issue. I only checked that its public Wikidata endpoint accepts the
{ } UNION { … }syntax.Alternatives I tried
Two shapes go further. Putting the required top-level properties in branches as well gives 1125 solutions, and one branch per property, each with the path from the root, gives 1292. On the same catalog the bare query took about 245ms for this PR’s shape, 210ms for the first alternative and 155ms for the second. Both change what happens when a required property is missing, so I kept the smaller change. If you would rather have one of them, I can rework this.
Tests
Two tests in
tests/e2e/optional.test.ts: one for entities with every, some and none of the optional properties, including a nested optional schema, and one that counts the quads the generated query returns. The second fails onmain.