Use native bucket scanner also in EditableMappingService

[fm3]

#8460 introduced the native bucket scanner, that collects the segment ids from a bucket.

This PR now uses that also in the EditableMappingService, which previously had its own implementation of collectSegments.

Steps to test:

• Proofreading should still work (split/merge results should make sense, also after reload)

---

• Removed dev-only changes like prints and application.conf edits
• Considered common edge cases
• Needs datastore update after deployment

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR expands and refactors the segment ID collection functionality across multiple components. A new Boolean parameter skipZeroes has been added to the collectSegmentIds method in both Scala and JNI code, modifying its signature and internal logic. Additionally, data handling in the tracingstore layers has been streamlined by switching from intermediate typed conversions to direct byte array processing, updating method signatures and visibility as needed.

Changes

File(s)	Summary of changes
webknossos-datastore/app/.../NativeBucketScanner.scala webknossos-jni/src/bucketScanner.cpp webknossos-jni/src/include/...NativeBucketScanner.h	Updated collectSegmentIds signature by adding the skipZeroes parameter and adjusted conditional logic in the JNI implementation.
webknossos-tracingstore/app/.../editablemapping/EditableMappingLayer.scala webknossos-tracingstore/app/.../editablemapping/EditableMappingService.scala	Modified data processing to use byte arrays directly for segment ID collection; updated method signatures, made helper methods/private, and introduced a new native scanner variable.
webknossos-tracingstore/app/.../volume/VolumeSegmentIndexService.scala	Included the additional skipZeroes parameter in the method call to collectSegmentIds, altering how segment IDs are filtered during processing.

Possibly related PRs

• If a split action doesn’t actually split the agglomerate, skip no-op steps #8488: Introduced similar modifications to the collectSegmentIds method by adding a parameter for skipping zero values.
• Fix merging editable mappings where self-merge is generated; fix compacted update action writes #8367: Also updated the collectSegmentIds signature to include an additional parameter, aligning with the changes in this PR.

Suggested labels

refactoring

Suggested reviewers

• MichaelBuessemeyer
• philippotto

Poem

I hop through fields of code so bright,
With skipZeroes guiding me just right.
Each method tweaked, each change so clear,
I nibble bugs and bring them near.
In every commit, my joy does flow,
A rabbit’s leap where clean codes grow! 🐇

✨ Finishing Touches

• 📝 Generate Docstrings

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai plan to trigger planning for file edits and PR creation.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (2)

113-113: Consider externalizing the path configuration.

Using a hardcoded path (Paths.get("")) may cause deployment mismatches. For improved maintainability, consider loading it from a configuration file or environment variable.

---

375-375: Avoid potential duplication with bytesToLongs.

bytesToSegmentInt is similar to bytesToLongs, so consider unifying both methods if possible to prevent code duplication and reduce maintenance overhead.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ef0222 and ddfa55d.

📒 Files selected for processing (6)

• webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/NativeBucketScanner.scala (1 hunks)
• webknossos-jni/src/bucketScanner.cpp (2 hunks)
• webknossos-jni/src/include/com_scalableminds_webknossos_datastore_helpers_NativeBucketScanner.h (1 hunks)
• webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala (1 hunks)
• webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (4 hunks)
• webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeSegmentIndexService.scala (1 hunks)

🧰 Additional context used

🧬 Code Definitions (4)

webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/NativeBucketScanner.scala (3)

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)

• collectSegmentIds (351-360)

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeSegmentIndexService.scala (1)

• collectSegmentIds (132-141)

webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (2)

• bytesPerElement (101-113)
• isSigned (67-82)

webknossos-jni/src/include/com_scalableminds_webknossos_datastore_helpers_NativeBucketScanner.h (1)

webknossos-jni/src/bucketScanner.cpp (1)

• jlongArray (60-88)

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeSegmentIndexService.scala (2)

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)

• collectSegmentIds (351-360)

webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (2)

• bytesPerElement (101-113)
• isSigned (67-82)

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (3)

webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/NativeBucketScanner.scala (1)

• NativeBucketScanner (5-11)

webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (3)

• ElementClass (45-189)
• bytesPerElement (101-113)
• isSigned (67-82)

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeSegmentIndexService.scala (1)

• collectSegmentIds (132-141)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: circleci_build

🔇 Additional comments (10)

webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/NativeBucketScanner.scala (1)

7-10: Method signature updated with a new parameter

The collectSegmentIds method now includes a new boolean parameter skipZeroes to control whether zero values should be included or excluded during segment ID collection. This change is consistent with the PR objective to use the native bucket scanner in EditableMappingService.

webknossos-jni/src/include/com_scalableminds_webknossos_datastore_helpers_NativeBucketScanner.h (1)

13-16: JNI header signature updated to match Scala implementation

The method signature has been updated from ([BIZ)[J to ([BIZZ)[J to accommodate the new skipZeroes parameter, ensuring consistency between the JNI interface and the Scala implementation. This signature change correctly reflects the parameter addition in the native implementation.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeSegmentIndexService.scala (1)

135-138: Parameter added to skip zero values in VolumeSegmentIndexService

The skipZeroes parameter is now set to true in this service, which will exclude zero values when collecting segment IDs. This is consistent with the implementation in the native bucket scanner and aligns with the PR objective.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala (3)

62-62: Streamlined segment ID collection using native scanner

The implementation now directly collects segment IDs from the unmapped data bytes rather than converting to a typed format first. This simplifies the code and leverages the native bucket scanner as intended in the PR.

---

68-68: Direct use of unmapped data for mapping

Similar to the segment ID collection change, the mapData method now directly uses the original unmapped data bytes instead of a converted typed format. This approach is more efficient and consistent with the PR's goal of utilizing the native bucket scanner throughout the EditableMappingService.

---

62-68: Verify data consistency after split and merge operations

The changes to how data is processed could potentially affect split and merge operations. According to the PR objectives, testing should include verification that split and merge results remain logical after service reload.

Could you confirm that you've tested split and merge operations after these changes and verified that the results remain consistent after reloading the service?

webknossos-jni/src/bucketScanner.cpp (1)

61-72: Parameter skipZeroes logic appears correct.

The added condition (!skipZeroes || currentValue != 0) accurately controls whether zero values should be skipped. The JNI signature update and the accompanying logic are consistent with the intended functionality.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (3)

15-20: Imports appear appropriate.

All newly added imports (e.g., NativeBucketScanner) are used and relevant. No issues to report.

Also applies to: 23-23

---

124-125: Lazy initialization of nativeBucketScanner.

Declaring nativeBucketScanner as lazy val is a sensible approach for on-demand usage, avoiding unnecessary initialization. No concerns here.

---

351-358: Double-check skipZeroes = false necessity.

This method always includes zero values when collecting IDs. Verify that including zero segment IDs is truly desired in all cases, or consider making this choice configurable.