Skip to content

Rjf/omf revisions#67

Merged
robfitzgerald merged 7 commits intomainfrom
rjf/omf-revisions
Dec 18, 2025
Merged

Rjf/omf revisions#67
robfitzgerald merged 7 commits intomainfrom
rjf/omf-revisions

Conversation

@robfitzgerald
Copy link
Collaborator

revisions to the OMF collection code:

  • was failing when running the bbox predicate which was written expecting only POINT types
  • refactored record deserialization to reduce duplication
  • some small logging improvements

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses a bbox predicate bug that was failing for non-POINT geometry types and refactors the record deserialization code to eliminate duplication and improve maintainability.

  • Fixed bbox predicate to handle all geometry types by using xmin/xmax/ymin/ymax instead of assuming POINT types
  • Consolidated duplicate deserialization logic by introducing a generic process_batch method and From trait implementations
  • Enhanced logging to include record type in collection messages

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
rust/bambam-omf/src/collection/record/record_type.rs Added generic process_batch method and Display trait implementation to reduce code duplication
rust/bambam-omf/src/collection/record/overture_record.rs Implemented From traits for all record types to enable automatic conversion
rust/bambam-omf/src/collection/filter/bbox_row_predicate.rs Fixed bbox predicate logic to check xmin/xmax/ymin/ymax for all geometry types, extracted helper functions
rust/bambam-omf/src/collection/collector.rs Refactored to use new process_batch method, eliminated duplicate deserialization code, improved logging

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

Comment on lines +28 to +29
/// tests the bounding box of each row in the record batch, filtering entries
/// that are not fully-contained.
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The comment says the filter checks for entries that are "fully-contained", but this should be clarified. The actual behavior depends on the implementation of the within_box function. If the intent is to check full containment of the row's bbox within the predicate's bbox, this should be stated more explicitly in the comment.

Copilot uses AI. Check for mistakes.
@robfitzgerald robfitzgerald merged commit 0c1ffef into main Dec 18, 2025
1 check passed
@robfitzgerald robfitzgerald deleted the rjf/omf-revisions branch December 18, 2025 23:35
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.

2 participants