[Aklakan]

GitHub issue resolved #3994

Pull request Description: Adds skip scan evaluation for single-pattern queries to TDB2. Added special code paths to OpExecutorTDB2 for OpDistinct and OpGroupBy.

Execution time to find all distinct predicates on Wikidata Truthy (8B triples) becomes between 1 and 100 seconds (warm-cold caches)
Also works for simple group by patterns such as
SELECT ?p (COUNT(DISTINCT ?g) AS ?c) { GRAPH ?g { ?s ?p ?o } } GROUP BY ?p

Need yet to add tests - such as create permutations of patterns and compare results with a reference engine (eg. ARQ).

---

• Tests are included.
• [ ] Documentation change and updates are provided for the Apache Jena website
• Commits have been squashed to remove intermediate development commit messages.
• Key commit messages start with the issue number (GH-xxxx)

By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.

---

See the Apache Jena "Contributing" guide.

[Aklakan]

The code is ready for review.

• The most critical changes to existing code are those in BPTreeDistinctKeyPrefixIterator which are meant to restrict skip scans / index scans to custom sub-ranges.
  
  When no custom range is specified, then the range used to be bptRootNode.{minRecord, maxRecord}, which indicates that maxRecord is inclusive for page-level iteration. However, when doing record-level interation using records.getRecordBuffer().iterator(minRecord, maxRecord) then maxRecord is exclusive. So this is something @rvesse and/or @afs might want to look at whether this is correct and how it should be.
• The class TestOpExecutorTDB2SkipScan generates SELECT DISTINCT ... and SELECT ... (COUNT(DISTINCT ?v) AS ?c) { ... } GROUP BY ... queries with different combinations and permutations.
• There is the SystemTDB.symSkipScan symbol to disable the optimization.

[Aklakan]

Endless loop because iterator is never advanced.
This method was used during development before settling on
iter = records.getRecordBuffer().iterator(minRecord, maxRecordPlusOne); in BPTreeDistinctKeyPrefixIterator.

[rvesse]

Hey @Aklakan this looks like a really great potential improvement. I had always thought what I'd done previously for prefix scanning had potential to be applied to other kinds of queries but never any chance to explore that further so thanks for taking this on

I have various comments across the PR, these covers three main things (plus the odd typo):

1. Whether this can/should handle other aggregates like MAX(DISTNCT ?var) since it would seem applying the distinct in the scan first wouldn't change their outputs either.
2. Improvements to testing, particularly around regression and scale
3. Leftover TODOs which may/may not remain relevant

[Aklakan]

I have completed my revision. The code is now more modular:

• OpExecutorTDB2 may try to perform skip scan execution for OpDistinct and OpGroup. It is aimed to work for simple queries such as
  • SELECT (min(?o) AS ?min) (max(?o) AS ?max) { ?s ?p ?o } and
  • SELECT DISTINCT ?p { ?s ?p ?o }
• SkipScanRewrite.isSkipScanCandidate restricts the op-structures eligible to skip scan execution is applied. The restriction could be relaxed in the future.
• TransformDistinctPlacement implements a transformer that pushes DISTINCT operations down - ideally adjacent to a single the triple pattern / quad. This transform could eventually go to ARQ as a general tool.
• OpExtSkipScan is a custom operator that effectively encapsulates (distinct (project (vars) (triple-or-quad)))
• TestOpExecutorTDB2SkipScan generates data of configurable size and all permutations of single-{triple/quad} access patterns and compares the result set against a reference in-memory dataset.

[rvesse]

I have completed my revision. The code is now more modular:

• OpExecutorTDB2 may try to perform skip scan execution for OpDistinct and OpGroup. It is aimed to work for simple queries such as

  • SELECT (min(?o) AS ?min) (max(?o) AS ?max) { ?s ?p ?o } and
  • SELECT DISTINCT ?p { ?s ?p ?o }

• SkipScanRewrite.isSkipScanCandidate restricts the op-structures eligible to skip scan execution is applied. The restriction could be relaxed in the future.

• TransformDistinctPlacement implements a transformer that pushes DISTINCT operations down - ideally adjacent to a single the triple pattern / quad. This transform could eventually go to ARQ as a general tool.

• OpExtSkipScan is a custom operator that effectively encapsulates (distinct (project (vars) (triple-or-quad)))

• TestOpExecutorTDB2SkipScan generates data of configurable size and all permutations of single-{triple/quad} access patterns and compares the result set against a reference in-memory dataset.

Thanks @Aklakan, I will aim to review this again later in the week

[afs]

@rvesse - how do the changes look to you?

[rvesse]

This looks good to me now.

Test coverage is substantially improved which was one of my requests from my original review. Have checked out the branch and ran the new test suite locally.

Couple of minor comments

[Aklakan]

I added the requested minor changes and had another look over the code and it still looked ok to me. So for now I'd say its done. In case a bug is revealed there is still SystemTDB.symSkipScan that can be set to false to disable this feature.

[afs]

To get up-to-speed with this PR, I did a AI code review (Claude Code, model=Opus5, effort=xhigh).
This was a bit of an experiment. Just because it finds something does not necessarily mean there is an issue.

It came up with some findings. I got it to generate test cases then verified some of them (1, 4 and 6 - they have test SELECT queries) by taking the data and query then using tdb2.tdbquery --mem=DATA --query=QUERY using the PR code base and also Jena main. The ones I looked at do behave differently.

I think some of findings are the same underlying cause in appearing in different ways.

tdb2:skipScan works.

I'm not sure then best way to exchange the material. If there is a better way, please say so.

I've put the material on a branch in my afs/jena repo.
https://github.com/afs/jena/tree/pr3995-tdb2-single-pattern-queries

The code is the PR, rebased to Jena main as of today.

3 commits:

1. The generated tests
2. The report
3. A fix needed for tdb2.tdbloader --mem=

Claude offered to "split into per-finding comments for posting inline on the PR (/code-review 3995 --comment does that)" if that would be helpful.

[Aklakan]

Thanks for the Claude analysis. All wrong SPARQL answers were so far fixable. I am currently down to 6 failing test cases which I still need to look into - at least 2 seem to be invalid.

• One conceptual issue is the min/max handling in BPTreeDistinctKeyPrefixIterator. Right now matching ranges uses an exclusive maximum - but perhaps it must be possible to match the maximum (0xffff...) value itself. Then the logic would have to be changed for the maximum to be inclusive, and an empty range would no longer be represented by [x, x[ but e.g. by [x, y] with y < x.

• The other thing I noticed is that TupleLib reads records just based on NodeId.SIZE, but in the comments NodeId can have two encodings: "long" with 8+4 bytes and "short" with 8 bytes. But NodeId.SIZE is only 8. So now I wonder whether the BPTree can actually mix different lengths of NodeIds - if that was possible something might be more fundamentally broken.

Test05_EqualityLinkArrayOverflow

• distinct_quadWithOneVarInEverySlot: SELECT DISTINCT ?s { ?s ?s ?s } Claude expects skip scan, but each row needs to be checked, so skip scan can't be used directly.
  It would be possible to rewrite the original query as below but I'd say this is out of scope for the PR.

SELECT ?s {
  { SELECT DISTINCT ?s { ?s [] [] } }
  FILTER EXISTS { ?s ?s ?s }
}

Test12_PrefixIteratorRange

• First impression is that singletonPageShortcutRespectsLowerBound is invalid: The test case inserts records [00000-00005] into the BPT and then does a retrieval with the range [00001-00005] but a prefix length of 3. So effectively a query for the range [000_00, 000_FF]. It then fails because the retrieval yields 00000 which is less than the minimum of the request 00001. But for the given prefix length 00000 is correct.

I still have to look into Test10_RemapValueConditions, Test11_KeyPrefixLengthValidation.

[afs]

• The other thing I noticed is that TupleLib reads records just based on NodeId.SIZE, but in the comments NodeId can have two encodings: "long" with 8+4 bytes and "short" with 8 bytes. But NodeId.SIZE is only 8. So now I wonder whether the BPTree can actually mix different lengths of NodeIds - if that was possible something might be more fundamentally broken.

Only "short" is implemented.

While NodeId is designed for 8+4, it has never been used. It woudl need testing out before use in a release as the implicit assumpt of 8 may well have leaked into some code or other.

It was never intended for mixed use. One database - one size of NodeId.

[afs]

• distinct_quadWithOneVarInEverySlot: SELECT DISTINCT ?s { ?s ?s ?s } Claude expects skip scan, but each row needs to be checked, so skip scan can't be used directly.
  It would be possible to rewrite the original query as below but I'd say this is out of scope for the PR.

Good plan.

Other reviewers have discovered edge cases where the new behaviour breaks

[Aklakan]

I'm not sure then best way to exchange the material. If there is a better way, please say so.

Good question - (Heh, sounds like an AI response). So the nice thing is that Claude isolated all changes - even private access - using the Internals class.

I am nearing completion of my fixes, the question is how to add them. perhaps i add only the fixes to this PR, and you rebase the Claude tests on top of them? So ideally the tests would eventually use e.g. package-private access rather than reflection via Internals - that would require removing some 'private' declarations.
Alternatively, I could include the tests in this PR - whatever you prefer.

[afs]

I am nearing completion of my fixes, the question is how to add them. perhaps i add only the fixes to this PR, and you rebase the Claude tests on top of them? So ideally the tests would eventually use e.g. package-private access rather than reflection via Internals - that would require removing some 'private' declarations.

Update the PR - pick any tests that are useful.

I can generate a new Claude review from scratch and it generate can tests based on the new state.

This is also an option

Claude offered to "split into per-finding comments for posting inline on the PR" if that would be helpful.

[Aklakan]

I added the fixes and the test cases in a new commit and ran a review using Astra/Medium which found 4 more issues in my changes for Claude which are all fixed now and verified with additional tests. (I did not squash them yet so that the AI can track progress in the commits).
I think its ready for another round of review on your side.
There are some disabled tests which have a comment why they are disabled - eventually those need to be cleaned up.

The following points were raised as potential code smells, but the design choices were deliberately that way for the given reasons (in case Claude also marks them):

• Executor stored in context: A workaround for OpExt.eval() not receiving the current executor. QC.execute() can create a different executor and break custom implementations. Retaining the exact instance is intentional; first-executor-wins remains a limitation.
• Tuple/index coordinate types: EqualityLink and VarMap serve both coordinate systems. Separate types could prevent remapping mistakes, but add complexity. Clear naming, documentation, and tests are sufficient for now.
• Equality checks through SPARQL expressions: Synthetic variables and filter/project iterators add overhead, but reuse established abort handling. Direct NodeId filtering is deferred unless performance warrants it.
• Cache ownership: The cache is execution-scoped, not global. Shared child contexts merit awareness, but no invalid physical-index reuse was demonstrated; no cache redesign is proposed.