Skip to content

feat(java): count indexed rows on explicit scalar segments - #9644

Open
hfutatzhanghb wants to merge 8 commits into
lance-format:mainfrom
hfutatzhanghb:codex/fix-indexed-row-count-segments
Open

hfutatzhanghb wants to merge 8 commits into
lance-format:mainfrom
hfutatzhanghb:codex/fix-indexed-row-count-segments

Conversation

@hfutatzhanghb

@hfutatzhanghb hfutatzhanghb commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

feat(java): count indexed rows on explicit scalar segments

Summary

  • Pin the existing Dataset.countIndexedRows method to the named scalar index, and reject filters that cannot be planned against that index. The three-argument signature is unchanged. This corrects the method so it counts with the index it was given, rather than scanning or selecting another index.
  • Add an overload that opens only the requested physical segment UUIDs. Their current fragment coverage defines the count scope, and an explicit fragment list must match that coverage. A selection that omits a fragment-reuse contributor for that scope is rejected; the error names the missing UUIDs.
  • Keep validation and count semantics in the Rust core. Deleted rows stay excluded, including on datasets that use stable row IDs.
  • The three-argument signature is unchanged and the segment overload is additive. This does not change a published method signature, so it should not be labeled as a public API break.

Test plan

  • ./mvnw spotless:apply and ./mvnw spotless:check in java/
  • ScalarIndexTest covers independent segment counts, A+B versus the full logical index, coverage mismatch, unknown and wrong-index UUIDs, duplicate and empty input, deletions, stable row IDs, and proof that an unselected segment file is not opened
  • Local Cargo compile and tests were not run on this machine. Remote CI is the compile and test gate.
  • Confirm existing three-argument Java callers still compile

Pin countIndexedRows to the named scalar index and add an overload that opens only the requested physical segments. Selected segment coverage defines the count scope and must match an explicit fragment list.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions github-actions Bot added enhancement New feature or request A-java Java bindings + JNI labels Oct 1, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

Match guards are not exhaustive, so Rust 1.91 rejected the segment loader. Format the new indexed-count path with the repository rustfmt rules.
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Oct 1, 2026
After fragment reuse, one source segment advertises every destination
fragment while still depending on its siblings. Require every contributor
for that scope so a partial UUID set cannot under-count.
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Oct 1, 2026
Callers need to see that a fragment-reuse scope is accepted only when every
contributing segment UUID is supplied.
lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Oct 1, 2026
@hfutatzhanghb

Copy link
Copy Markdown
Contributor Author

Hi, @Xuanwo @wjones127 @BubbleCal Could you please help review this PR when have free time? Thanks very much!

@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Oct 2, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Oct 2, 2026
Legacy datasets count through MaterializeIndexExec, which opened every
segment of the logical index. Pass the requested UUIDs into that loader.
@lance-gatekeeper lance-gatekeeper Bot removed the K-approved Latest Gatekeeper recommendation permits acceptance. label Oct 2, 2026

@lance-gatekeeper lance-gatekeeper Bot 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.

✅ Gate recommendation: approve.

Named-index counts now respect UUID selection on both modern and legacy storage, while retaining deletion filtering and the complete-contributor requirement after fragment reuse. Both earlier findings remain fixed.

The public Rust open_scalar_index_segments signature gains a parameter; the Java overload is additive. Please mark this PR with the breaking-change label.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Oct 2, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-java Java bindings + JNI enhancement New feature or request K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant