feat: Add optional alignment tag support to BAM table provider

[mwiewior]

Summary

Implements lazy parsing of BAM alignment tags (NM, MD, AS, etc.) as queryable columns, following the VCF INFO/FORMAT field pattern established in #46.

Changes

Core Implementation

• Tag Registry (tag_registry.rs): Registry of ~40 common BAM tags with SAM type → Arrow type mappings
• Table Provider Updates: Added tag_fields: Option<Vec<String>> parameter for explicit tag specification
• Physical Execution: Implemented lazy tag parsing with projection pushdown
• Metadata Support: Added BAM tag metadata constants to core library

Key Features

• ✅ Only parses tags when explicitly requested in SQL projections (lazy evaluation)
• ✅ Support for all SAM value types: integers, floats, strings, hex, character, arrays
• ✅ Comprehensive error handling with helpful messages listing available tags
• ✅ Registry includes alignment scoring (NM, MD, AS, XS), read groups (RG, LB, PU), single-cell barcodes (CB, UB, UR, CR), quality scores (BQ, OQ), and more
• ✅ Follows existing VCF pattern for consistency

Testing

• Unit tests for tag registry (type mapping, coverage, descriptions)
• Integration tests for schema extension, projection, filtering, aggregations
• Example program demonstrating various query patterns
• Updated benchmark runner to use new API

Documentation

• Updated BAM README with tag usage examples
• Added schema documentation for optional tags
• Code examples for basic and tag-enabled queries

Usage Example

// Create BAM table with alignment tags
let provider = BamTableProvider::new(
    "data/alignments.bam".to_string(),
    Some(4),  // threads
    None,     // storage options
    true,     // 0-based coordinates
    Some(vec!["NM".to_string(), "MD".to_string(), "AS".to_string()]),
)?;

// Query with tags
let df = ctx.sql("
    SELECT name, chrom, start, \"NM\", \"AS\"
    FROM alignments
    WHERE \"NM\" <= 2 AND \"AS\" >= 100
").await?;

Performance

Tags are only parsed when projected in queries, ensuring no performance impact when tags are defined but not used.

🤖 Generated with Claude Code

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[mwiewior]

@claude pls review this pr for any code duplicates, dead code or performance improvements.

[mwiewior]

@claude pls review this pr for any code duplicates, dead code or performance improvements.

[claude]

Claude finished @mwiewior's task in 1m 28s —— View job

---

Code Review: BAM Optional Tags Implementation

Reviewing for code duplicates, dead code, and performance improvements.

Todo List

• Read and analyze key implementation files
• Check for code duplication
• Identify dead code
• Analyze performance characteristics
• Compare with VCF implementation pattern
• Provide comprehensive feedback

---

Review Summary

Overall, this is a solid implementation that follows good patterns for lazy tag parsing. The code is well-structured and the approach is sound. However, I've identified several areas for improvement:

---

🔴 Critical Issues

1. Significant Code Duplication in physical_exec.rs

The get_remote_bam_stream() and get_local_bam() functions contain massive duplication (~90% identical code, 350+ lines duplicated). Lines 241-426 vs lines 428-611 are nearly identical except for the reader initialization.

Location: datafusion/bio-format-bam/src/physical_exec.rs:241-611

Impact: Maintenance burden, potential for bugs when one function is updated but not the other.

Recommendation: Extract shared logic into a common function. Consider a structure like:

async fn process_bam_records<S: Stream<Item = Result<Record>>>(
    records: S,
    schema: SchemaRef,
    batch_size: usize,
    // ... other params
) -> Result<RecordBatchStream>

This would reduce code by ~300 lines and eliminate the duplication.

Fix this →

---

🟡 Dead Code & Unused Attributes

2. Multiple #[allow(dead_code)] attributes

Several functions are marked as dead code:

• tag_registry.rs:9-10 - TagDefinition.tag field
• tag_registry.rs:412 - sam_tag_type_to_arrow_type() function
• tag_registry.rs:426 - infer_type_from_noodles_value() function
• physical_exec.rs:29 - BamExec struct

Analysis:

• TagDefinition.tag - genuinely unused (redundant since it's the HashMap key)
• sam_tag_type_to_arrow_type() - genuinely unused in production code (only in tests implicitly)
• infer_type_from_noodles_value() - unused, but potentially useful for future dynamic type inference
• BamExec dead_code - likely a false positive due to the any() pattern

Recommendations:

1. Remove TagDefinition.tag field entirely (it's the HashMap key already)
2. Keep utility functions for now if they're part of the public API, otherwise remove them
3. The BamExec warning is probably unavoidable due to the as_any() pattern

Fix this →

---

⚡ Performance Observations

3. Tag Parsing Efficiency

Good:

• ✅ Lazy evaluation is correctly implemented - tags only parsed when projected
• ✅ Tag builders are only initialized if tag_fields is Some
• ✅ Proper use of Vec::with_capacity() for pre-allocation

Potential Issue:
The tag lookup happens per record with string parsing:

// physical_exec.rs:134-139
let tag_bytes = tag_name.as_bytes();
if tag_bytes.len() != 2 {
    builder.append_null()?;
    continue;
}
let tag = Tag::from([tag_bytes[0], tag_bytes[1]]);

Recommendation: Pre-parse tags once in set_tag_builders() instead of parsing tag names on every record. Store Vec<Tag> alongside tag names:

type TagBuilders = (Vec<String>, Vec<DataType>, Vec<OptionalField>, Vec<Tag>);

This would eliminate ~N_records × N_tags string-to-tag conversions.

Fix this →

---

4. Array Iteration Pattern

In multiple places, there's repetitive array conversion code (lines 172-226 in physical_exec.rs):

SamArray::Int8(vals) => {
    let vec: Result<Vec<i32>, _> = vals.iter().map(|v| v.map(|x| x as i32)).collect();
    match vec {
        Ok(v) => builder.append_array_int(v)?,
        Err(_) => builder.append_null()?,
    }
}
// ... repeated 6 times with different types

Recommendation: Extract to a helper function or macro to reduce repetition:

fn convert_array_to_i32<T: Into<i32>>(vals: &Array<T>) -> Result<Vec<i32>, Error> {
    vals.iter().map(|v| v.map(Into::into)).collect()
}

---

🟢 Good Practices Observed

1. ✅ Consistent error handling with helpful messages (e.g., listing available tags)
2. ✅ Good test coverage with unit tests and integration tests
3. ✅ Proper metadata usage for tag information in schema
4. ✅ Documentation is thorough with examples
5. ✅ Follows VCF pattern for consistency across the codebase
6. ✅ Type safety with proper SAM type → Arrow type mappings

---

📝 Minor Suggestions

5. Commented Code

physical_exec.rs:451 has commented let mut count: usize = 0; - should be removed.

6. Schema Metadata Consistency

The coordinate system metadata is added at schema level, while tag metadata is at field level. This is fine, but consider documenting why this design choice was made.

7. Builder Pattern Complexity

The TagBuilders type alias is a 3-tuple (or 4-tuple with my suggestion). Consider a dedicated struct for better readability:

struct TagBuilderState {
    names: Vec<String>,
    types: Vec<DataType>,
    builders: Vec<OptionalField>,
    parsed_tags: Vec<Tag>,  // if optimization #3 is applied
}

---

Performance Impact Summary

✅ No performance regression for existing users (tags=None)
✅ Lazy evaluation ensures no overhead when tags aren't queried
⚠️ Room for optimization in hot path (tag name parsing per record)

Estimated impact of suggested optimizations:

• Code duplication fix: No performance change, but -300 lines of code
• Tag pre-parsing: Could save 5-10% on tag-heavy queries
• Array conversion helpers: Readability improvement, negligible perf impact

---

Conclusion

This PR implements a valuable feature with a solid architectural approach. The main issue is code duplication in the stream processing functions, which should be addressed before merge. The dead code warnings are mostly benign but worth cleaning up. Performance is good with opportunities for minor optimizations.

Recommendation: Address the code duplication issue (#1) as it significantly impacts maintainability. Other suggestions are lower priority but would improve code quality.

---

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[mwiewior]

@claude pls review the update.

[claude]

Claude finished @mwiewior's task in 1m 34s —— View job

---

Code Review: Updated BAM Optional Tags Implementation

Reviewing changes in commit 610c0c9 that address previous feedback.

Todo List

• Read updated physical_exec.rs to check code duplication fix
• Read tag_registry.rs to check dead code cleanup
• Review git diff to see what changed
• Provide comprehensive updated review feedback

---

Review Summary

Excellent work addressing the previous feedback! All three major issues have been successfully resolved. The refactoring is clean and the code quality has significantly improved.

---

✅ Issues Resolved

1. Code Duplication Eliminated (Critical Issue #1)

Status: ✅ FIXED

The massive code duplication between get_remote_bam_stream() and get_local_bam() has been completely eliminated using a well-designed macro solution.

What was done:

• Created process_bam_records_impl! macro (lines 238-444 in physical_exec.rs)
• Extracted all shared BAM record processing logic into the macro
• Both get_remote_bam_stream() and get_local_bam() now simply call the macro with appropriate parameters
• Reduced code by ~350 lines

Quality of solution:

• ✅ Macro is well-documented with clear parameter descriptions
• ✅ Uses try_stream! hygienically
• ✅ All variables properly parameterized with $ prefix
• ✅ Both functions now reduced to ~20 lines each (down from ~200 lines)
• ✅ Logic is identical between local and remote paths, ensuring consistency

Example (lines 446-467):

async fn get_remote_bam_stream(...) -> ... {
    let mut reader = BamReader::new(file_path.clone(), None, object_storage_options).await;
    let stream = process_bam_records_impl!(
        reader, schema, batch_size, projection,
        coordinate_system_zero_based, tag_fields
    );
    Ok(stream)
}

This is an excellent refactoring that maintains readability while eliminating duplication.

---

2. Dead Code Warnings Cleaned Up (Issue #2)

Status: ✅ FIXED

The redundant tag field in TagDefinition struct has been removed.

What was done:

• Removed tag: String field from TagDefinition struct (tag_registry.rs:8-10)
• Removed all 40+ redundant tag: "XX".to_string() assignments from get_known_tags()
• Removed the #[allow(dead_code)] attribute

Why this is correct:

• The tag name is already the HashMap key in get_known_tags(), so storing it again in the value was redundant
• Reduced struct size and memory usage
• Cleaner code with less duplication

Example change (lines 29-36):

// Before:
TagDefinition {
    tag: "NM".to_string(),    // redundant!
    sam_type: 'i',
    arrow_type: DataType::Int32,
    description: "Edit distance to the reference".to_string(),
}

// After:
TagDefinition {
    sam_type: 'i',
    arrow_type: DataType::Int32,
    description: "Edit distance to the reference".to_string(),
}

---

3. Tag Parsing Performance Optimized (Performance Issue #3)

Status: ✅ FIXED

Tag parsing has been moved from the hot path (per-record) to initialization time.

What was done:

• Changed TagBuilders type from 3-tuple to 4-tuple, adding Vec<Tag> (line 104)
• Modified set_tag_builders() to pre-parse tags once during initialization (lines 118-124)
• Updated load_tags() to use pre-parsed tags instead of parsing on every record (lines 133-136)

Performance impact:

• Before: N_records × N_tags tag parsing operations (string → Tag conversion on every record)
• After: N_tags tag parsing operations (once at initialization)
• Estimated improvement: 5-15% on tag-heavy queries with many records

Code comparison:

// Before (lines 132-139 in old code):
for i in 0..tag_builders.0.len() {
    let tag_name = &tag_builders.0[i];
    let builder = &mut tag_builders.2[i];
    let tag_bytes = tag_name.as_bytes();
    if tag_bytes.len() != 2 {  // ❌ Parsing EVERY record
        builder.append_null()?;
        continue;
    }
    let tag = Tag::from([tag_bytes[0], tag_bytes[1]]);  // ❌ Conversion EVERY record
    ...
}

// After (lines 133-136 in new code):
for i in 0..tag_builders.0.len() {
    let builder = &mut tag_builders.2[i];
    let tag = &tag_builders.3[i]; // ✅ Use pre-parsed tag (just a reference!)
    let data = record.data();
    let tag_result = data.get(tag);
    ...
}

This is exactly the optimization suggested in the previous review.

---

🟢 Additional Improvements Observed

4. Code Formatting and Comments

The refactoring also improved code organization:

• Better comment structure with clear section headers ("Extract record name", "Extract chromosome name", etc.)
• Consistent spacing and formatting throughout
• Removed commented-out dead code (let mut count: usize = 0; at line 451)
• Improved variable naming (e.g., chrom_name vs mate_chrom_name to avoid confusion)

---

📊 Impact Analysis

Lines of code reduced: ~350 lines (from ~680 to ~330 in physical_exec.rs)

Maintainability: Significantly improved

• Single source of truth for BAM record processing logic
• Changes only need to be made in one place (the macro)
• Eliminates risk of divergence between local and remote paths

Performance: Improved

• Tag parsing optimization eliminates redundant work
• No performance regression for non-tag queries
• Better cache locality with pre-parsed tags

Code quality: Excellent

• Well-documented macro with clear parameter descriptions
• Clean abstraction that doesn't hide complexity
• Type-safe with proper error propagation

---

🎯 Remaining Observations

The two utility functions still have #[allow(dead_code)] attributes:

• sam_tag_type_to_arrow_type() (line 370)
• infer_type_from_noodles_value() (line 384)

These are likely kept for potential future use or as part of the public API. If they're truly unused and not part of the public interface, they could be removed, but this is a minor point and not a blocker.

The #[allow(dead_code)] on BamExec (line 29) remains, which is expected due to the as_any() pattern used in ExecutionPlan implementations.

---

✨ Final Assessment

Status: ✅ All major issues addressed

The refactoring successfully addresses all three critical/important issues from the previous review:

1. ✅ Code duplication eliminated via macro abstraction
2. ✅ Dead code cleaned up (redundant tag field removed)
3. ✅ Performance optimized (tag pre-parsing implemented)

The implementation is clean, well-documented, and follows Rust best practices. The macro solution for code deduplication is particularly elegant - it maintains type safety while eliminating duplication without adding runtime overhead.

Recommendation: ✅ Ready to merge

The PR now has excellent code quality with no significant issues remaining. The optional tag support is well-implemented, performant, and maintainable.

---

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[mwiewior]

@claude pls review the updated PR

[claude]

Claude finished @mwiewior's task —— View job

---

Code Review: Updated BAM Optional Tags PR

Reviewing the latest changes to the PR (commits 6b1a6fc through d7ac1a8).

Todo List

• Check git log for recent commits since last review
• Review git diff to understand what changed
• Read and analyze new describe() method implementation
• Check mapping_quality nullability fix
• Review clippy warning fixes
• Provide comprehensive feedback

---

Review Summary

The PR continues to improve with four excellent commits since the last review. The new describe() method is a valuable addition that enables dynamic schema discovery. All fixes are technically sound and improve code quality.

---

✅ Improvements Since Last Review

1. New Feature: describe() Method (commit 6b1a6fc)

Status: ✅ Excellent addition

A schema introspection method that samples BAM records to discover available tags dynamically. This is very useful for exploring unfamiliar BAM files.

Key features:

• Samples N records (default 100) to discover tags
• Returns a DataFrame with column metadata (name, type, nullable, category, SAM type, description)
• Uses the tag registry for descriptions when available
• Well-documented with comprehensive examples

Quality observations:

• ✅ Proper error handling
• ✅ Clean separation of core vs tag columns
• ✅ Helpful categorization (core/tag)
• ✅ Good example program demonstrating usage patterns
• ✅ Integration tests added

Example output structure:

column_name | data_type | nullable | category | sam_type | description
------------|-----------|----------|----------|----------|-------------
name        | Utf8      | true     | core     | null     | Read name...
NM          | Int32     | true     | tag      | i        | Edit distance...

---

2. Clippy Warnings Fixed (commit b30e631)

Status: ✅ Clean fixes

Addressed three clippy warnings with idiomatic Rust improvements:

Changes:

// Before: Manual iteration with if-let
for result in data.iter() {
    if let Ok((tag, value)) = result {
        // ...
    }
}

// After: Idiomatic filter_map
for (tag, value) in data.iter().filter_map(Result::ok) {
    // ...
}

// Before: Check-then-insert pattern
if !discovered_tags.contains_key(&tag_str) {
    discovered_tags.insert(tag_str, ...);
}

// After: Idiomatic entry API
discovered_tags
    .entry(tag_str)
    .or_insert_with(|| infer_type_from_noodles_value(&value));

These changes improve both performance (no double HashMap lookup) and readability.

---

3. Tag Description Simplification (commit 9559487)

Status: ✅ Good cleanup

Removed redundant "Discovered in data." prefix from tag descriptions, making output cleaner and more concise. Now uses registry descriptions directly for known tags.

Before:

description: "Discovered in data. Edit distance to the reference"

After:

description: "Edit distance to the reference"

---

4. mapping_quality Nullability Fix (commit d7ac1a8)

Status: ✅ Correct fix

The fix:
Changed mapping_quality field from nullable to non-nullable in both determine_schema() and describe().

Why this is correct:
According to the SAM/BAM specification, MAPQ (mapping quality) is a required field that must always be present. The value ranges from 0-255, where 255 means "unavailable/unknown". Since there's a sentinel value (255) for unknown quality, the field itself should never be null.

Locations fixed:

• table_provider.rs:36 - schema definition
• table_provider.rs:296 - describe() output

This ensures schema consistency between the actual table schema and the describe() output.

---

🟡 Minor Observations

1. Dead Code Cleanup

The infer_type_from_noodles_value() function previously had #[allow(dead_code)] (from commit 610c0c9). This function is now actively used by the describe() method, so the warning is resolved organically. Good code reuse!

Usage: table_provider.rs:213, 251

---

2. Commented todo!() Markers

There are still commented todo!() markers in the code:

Location: table_provider.rs:404, 413

fn as_any(&self) -> &dyn Any {
    self
    // todo!()
}

fn table_type(&self) -> TableType {
    TableType::Base
    // todo!()
}

Recommendation: Remove these commented lines - they're implemented and working. The comments serve no purpose and create confusion.

Fix this →

---

3. Potential Type Inconsistency Detection

The describe() method uses or_insert_with() when discovering tags, which means only the first occurrence of each tag determines its type:

Location: table_provider.rs:249-251

discovered_tags
    .entry(tag_str)
    .or_insert_with(|| infer_type_from_noodles_value(&value));

Potential issue: If a tag appears with inconsistent types across records (e.g., tag "XX" is Int32 in record 1 but String in record 2), only the first type is recorded. Subsequent occurrences with different types are silently ignored.

Is this a real problem?

• Probably not in practice - the BAM spec requires consistent tag types within a file
• Well-formed BAM files won't have this issue
• The existing registry enforces expected types for known tags

Recommendation: This is acceptable as-is for now, but consider adding type validation in the future. A production-hardened version might:

1. Track type consistency and warn on conflicts
2. Use the registry as the source of truth for known tags (override inferred types)
3. Add a strict mode that errors on type inconsistencies

Not a blocker, just something to be aware of.

---

4. FIXME Comment from Earlier

There's still a FIXME comment from before this PR:

Location: table_provider.rs:33

Field::new("flags", DataType::UInt32, false), //FIXME:: optimize storage

This is unrelated to this PR and can be addressed separately. Just noting it for completeness.

---

🟢 Excellent Practices Observed

1. ✅ Incremental improvements - Each commit is focused and does one thing well
2. ✅ Responsive to tooling - Clippy warnings addressed promptly
3. ✅ Good documentation - The describe() method has excellent doc comments with examples
4. ✅ Test coverage - New functionality has corresponding integration tests
5. ✅ Practical examples - The describe_bam_schema.rs example shows real usage patterns
6. ✅ Consistency - describe() output aligns with actual schema structure
7. ✅ Code reuse - Leveraged existing infer_type_from_noodles_value() instead of duplicating logic

---

📊 Code Quality Metrics

Since last review:

• Lines added: ~240 (describe() implementation + tests + examples)
• Lines removed: ~12 (cleanups and refactoring)
• Clippy warnings fixed: 3
• New tests: 3 integration tests
• New examples: 1 comprehensive example
• Documentation: Excellent (full doc comments with examples)

Overall PR stats:

• Files changed: 15
• Lines added: 1,726
• Lines removed: 259
• Net improvement: Strong positive

---

🎯 Recommendations

High Priority:

• None - all critical issues from previous reviews have been addressed

Low Priority:

1. Remove commented todo!() markers at lines 404 and 413
2. Consider adding type consistency validation in describe() (future enhancement)

Nice to have:

• Consider making describe() check the registry first for known tags before inferring types

---

✨ Final Assessment

Status: ✅ Excellent updates, ready to merge

All four commits represent meaningful improvements:

• 6b1a6fc: Adds valuable schema discovery functionality
• b30e631: Improves code quality with idiomatic Rust patterns
• 9559487: Enhances user experience with cleaner output
• d7ac1a8: Fixes schema correctness per BAM spec

The PR is in excellent shape. The describe() method is a particularly nice addition that will help users explore unfamiliar BAM files. The implementation is clean, well-tested, and follows the established patterns in the codebase.

The only minor issue is the commented todo!() markers, which should be removed but are not blocking.

Great work on this feature!

---

[github-actions]

📊 Benchmark Results

Benchmarks have been completed and stored for this PR.

View Results: https://biodatageeks.org/datafusion-bio-formats/benchmark-comparison/

• Target: feat/bam-optional-tags
• Baseline: v0.1.1
• Platforms: Linux, macOS
• Mode: fast

Raw data: https://biodatageeks.org/datafusion-bio-formats/benchmark-data/

[claude]

The mapping_quality field is declared as non-nullable in the schema, but the data processing code in physical_exec.rs still produces null values for this field.

In the schema definition, the field was changed from nullable (true) to non-nullable (false):

datafusion-bio-formats/datafusion/bio-format-bam/src/table_provider.rs

Lines 33 to 38 in 8825e81

	Field::new("end", DataType::UInt32, true),
	Field::new("flags", DataType::UInt32, false), //FIXME:: optimize storage
	Field::new("cigar", DataType::Utf8, false),
	Field::new("mapping_quality", DataType::UInt32, false),
	Field::new("mate_chrom", DataType::Utf8, true),
	Field::new("mate_start", DataType::UInt32, true),

However, in the processing code, None is still pushed into the mapping_quality vector when record.mapping_quality() returns None:

datafusion-bio-formats/datafusion/bio-format-bam/src/physical_exec.rs

Lines 322 to 330 in 8825e81

	// Extract mapping quality
	match record.mapping_quality() {
	Some(mapping_quality_value) => {
	mapping_quality.push(Some(mapping_quality_value.get() as u32));
	},
	_ => {
	mapping_quality.push(None);
	}
	};

BAM records can have missing mapping quality (value 255 in the SAM spec means "unavailable"), which is common for unmapped reads. When such a record is encountered, Arrow's RecordBatch::try_new will fail validation because a null value exists in a non-nullable column.

This field should remain nullable as it was before this PR.