Add Variant SELECT rewriter and comprehensive architecture docs

[tonyalaribe]

Summary

• VariantSelectRewriter: Adds analyzer rule to automatically wrap Variant columns with variant_to_json() in SELECT projections, ensuring proper serialization for PostgreSQL wire protocol
• Comprehensive Documentation: Adds three new architecture docs covering the entire system
• Database improvements: Unified table storage model and DML integration updates

Documentation Added

• docs/ARCHITECTURE.md - Full system architecture overview with diagrams

  • Module structure
  • Data flow (Insert/Select/Flush paths)
  • Multi-tenant storage model
  • Key data structures (MemBuffer, WAL, Config)
  • Query transformation pipeline
  • Caching and safety mechanisms

• docs/VARIANT_TYPE_SYSTEM.md - Variant type implementation

  • Struct representation
  • INSERT/SELECT automatic transformations
  • JSON path operators (-> and ->>)
  • UDFs and usage examples

• docs/WAL.md - Write-ahead log implementation

  • Entry format and structure
  • Topic partitioning
  • Recovery process
  • Safety features

Code Changes

• src/optimizers/variant_select_rewriter.rs - New analyzer rule
• src/database.rs - Unified table storage improvements
• src/dml.rs - Buffered layer integration
• Schema and test updates

Test plan

• Run cargo test for unit tests
• Run cargo test --test integration_test for integration tests
• Verify Variant column SELECT returns JSON strings via psql

[claude]

PR Review: Add Variant SELECT rewriter and comprehensive architecture docs

Summary

This is a well-structured PR that adds critical functionality for Variant type handling and significantly improves documentation. The implementation is thoughtful and addresses PostgreSQL wire protocol compatibility correctly.

✅ Strengths

Code Quality

1. Excellent Variant Rewriter Design (src/optimizers/variant_select_rewriter.rs, variant_insert_rewriter.rs)

   • Clean separation of concerns with dedicated analyzer rules
   • Proper handling of already-wrapped expressions (prevents double-wrapping at line 54-58)
   • Preserves column aliases correctly (line 66-79 in select_rewriter.rs)
   • Good use of debug logging for observability

2. Robust Database Refactoring (src/database.rs)

   • Unified table storage model simplifies multi-tenancy
   • Proper error handling with exponential backoff (line 552)
   • Safe conversion between JSON strings and Variant with detailed error messages (line 168)
   • Consistent use of Arc/RwLock for thread safety

3. Configuration Improvements (src/config.rs)

   • New unified TIMEFUSION_DATA_DIR consolidates WAL and cache paths
   • Clean helper methods wal_dir() and cache_dir() (lines 240-246)
   • Good defaults and validation (e.g., min enforcement at line 440)

4. Comprehensive Documentation

   • ARCHITECTURE.md: Excellent system overview with clear diagrams
   • VARIANT_TYPE_SYSTEM.md: Well-documented automatic transformations
   • WAL.md: Thorough explanation of durability guarantees

Best Practices

• Proper use of DataFusion's TreeNode API for AST transformation
• Fail-fast approach on invalid JSON (database.rs:168)
• No backwards-compatibility hacks (clean removal of WALRUS_DATA_DIR and TIMEFUSION_FOYER_CACHE_DIR)

🔍 Issues & Recommendations

Security Concerns

HIGH: Credentials in Database Schema (database.rs:570-571)

s3_access_key_id VARCHAR(500) NOT NULL,
s3_secret_access_key VARCHAR(500) NOT NULL,

Storing credentials in plaintext is a security vulnerability. Consider:

• Encrypting credentials at rest using PostgreSQL's pgcrypto extension
• Using AWS IAM roles or STS temporary credentials instead
• Referencing AWS Secrets Manager or similar secret stores

MEDIUM: Potential SQL Injection in Dynamic Queries
While I don't see direct issues in this PR, ensure that any dynamic SQL construction (especially in DML operations) properly uses parameterized queries.

Code Quality Issues

1. Missing Error Context (variant_insert_rewriter.rs:32)

plan.transform_up(|node| rewrite_insert_node(node)).map(|t| t.data)

Consider adding error context to help with debugging:

plan.transform_up(|node| rewrite_insert_node(node))
    .map(|t| t.data)
    .map_err(|e| DataFusionError::Plan(format!("VariantInsertRewriter failed: {}", e)))

2. Recursive Transformation Could Stack Overflow (variant_insert_rewriter.rs:94-104)
The recursive call in rewrite_input_for_variant could overflow on deeply nested plans. Consider:

• Adding a depth limit
• Using an iterative approach with a stack
• Adding a comment about expected maximum depth

3. Magic Number (optimizers/mod.rs:29)

let date_scalar = ScalarValue::Date32(Some(date.and_hms_opt(0, 0, 0).unwrap().and_utc().timestamp() as i32 / 86400));

The division by 86400 (seconds per day) should be a named constant:

const SECONDS_PER_DAY: i32 = 86400;

4. Unused Code Warning (optimizers/mod.rs:7-10)

// Remove unused imports warning - these are used by the submodules indirectly
use datafusion::logical_expr::{BinaryExpr, Expr, Operator};
use datafusion::scalar::ScalarValue;

These imports ARE used directly in the time_range_partition_pruner module below. The comment is misleading - just remove it.

Performance Considerations

1. Cloning in Hot Path (variant_select_rewriter.rs:40)

expr.clone()

While necessary for the TreeNode pattern, consider documenting that this is acceptable because:

• DataFusion expressions use Arc internally
• Clones are shallow for most expression types
• The optimizer runs once per query, not per row

2. Batch Conversion Efficiency (database.rs:100-152)
The convert_variant_columns function processes columns sequentially. For large batches with multiple Variant columns, consider parallel conversion using rayon.

Test Coverage

GOOD:

• Integration test updates show the feature is tested end-to-end
• Test configuration properly isolates tests with unique IDs

NEEDS IMPROVEMENT:

• No dedicated unit tests for the new rewriter modules
• Missing test cases:
  • Double-wrapping prevention (already wrapped columns)
  • Nested projections with Variant columns
  • INSERT with partial column lists (columns beyond batch length)
  • Invalid JSON error handling path

Recommendation: Add unit tests in src/optimizers/variant_insert_rewriter.rs and variant_select_rewriter.rs:

#[cfg(test)]
mod tests {
    use super::*;
    use datafusion::prelude::*;

    #[test]
    fn test_variant_select_no_double_wrap() {
        // Test that already-wrapped columns aren't wrapped again
    }

    #[test]
    fn test_variant_insert_with_partial_columns() {
        // Test INSERT with fewer columns than table schema
    }
}

Documentation

Minor Issues:

1. VARIANT_TYPE_SYSTEM.md line 70: "Applies recursively to Values and Projection nodes" - Should clarify what happens with subqueries
2. ARCHITECTURE.md: Missing information about how Variant types interact with the buffered write layer

🎯 Action Items

Before Merge (Required):

1. ✅ Address the credentials storage security concern (use encryption or external secret management)
2. Add unit tests for the rewriter modules
3. Remove misleading comment in optimizers/mod.rs:7

Post-Merge (Recommended):

1. Add depth limit to recursive rewrite_input_for_variant
2. Extract magic number (86400) to named constant
3. Consider parallel Variant column conversion for large batches
4. Add more edge case tests for Variant handling

📊 Metrics

• Files changed: 24
• Additions: 1,839 lines (including 912 lines of documentation)
• Deletions: 264 lines
• Net impact: Significant improvement in code organization and documentation

Conclusion

This is high-quality work that significantly improves the Variant type system and documentation. The security concern around credential storage should be addressed before merging, but otherwise the implementation is solid. The new documentation is comprehensive and will help onboard contributors.

Recommendation: Approve with requested changes to credential storage

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

[claude]

Code Review: PR #16 - Variant SELECT/INSERT Rewriter and Architecture Docs

Overview

This is a well-architected and production-ready PR that adds comprehensive Variant type support, extensive documentation, and performance optimizations. The implementation is solid with clean separation of concerns and proper integration into the DataFusion query pipeline.

Overall Assessment: ✅ Approve with minor recommendations

---

🎯 Key Strengths

1. Clean Implementation

• Variant rewriters properly integrated into analyzer rule pipeline with correct ordering
• VariantInsertRewriter runs BEFORE TypeCoercion (prevents type mismatches)
• VariantSelectRewriter runs AFTER TypeCoercion (ensures correct output types)
• Double-wrap prevention implemented correctly in both rewriters

2. Comprehensive Documentation

• docs/ARCHITECTURE.md: Excellent system overview with clear diagrams
• docs/VARIANT_TYPE_SYSTEM.md: Well-explained Variant representation and transformations
• docs/WAL.md: Thorough coverage of WAL implementation and recovery

3. Performance Focus

• New benchmark suite covers critical paths (write, read, S3 flush/read)
• Compact-on-read optimization for MemBuffer queries
• Configurable bloom filters and WAL fsync intervals
• Proper retry/timeout configuration for S3 operations

4. Type Safety & Error Handling

• Proper use of Result types throughout
• JSON validation fails fast on invalid input
• WAL corruption tracking with configurable thresholds
• 100MB batch size limit prevents OOM attacks

---

⚠️ Areas for Improvement

High Priority

1. Missing Unit Tests for Variant Rewriters

Location: src/optimizers/variant_insert_rewriter.rs & variant_select_rewriter.rs

The rewriters have no dedicated unit tests - only indirect coverage through integration tests.

Recommendation: Add unit tests for:

// Test cases needed:
- Double-wrap prevention (SELECT with already-wrapped columns)
- Alias preservation in VariantSelectRewriter
- Nested expression handling in VariantInsertRewriter
- Edge case: Empty projections (COUNT(*))
- Edge case: Mixed Variant/non-Variant columns

2. Schema Evolution Not Addressed

Location: src/schema_loader.rs:18-26

The Variant type detection relies on exact struct pattern matching (metadata + value fields), but doesn't handle schema evolution:

pub fn is_variant_type(dt: &DataType) -> bool {
    matches!(dt, DataType::Struct(fields)
        if fields.len() == 2
        && fields.iter().any(|f| f.name() == "metadata")
        && fields.iter().any(|f| f.name() == "value"))
}

Issues:

• If JSON structure within Variant changes (new fields added/removed), no migration path
• If Parquet Variant spec evolves, detection could fail silently

Recommendation:

• Document Parquet Variant version dependency explicitly
• Consider adding version field or more flexible matching
• Add migration framework for future schema changes

3. JSON Conversion Error Handling

Location: src/database.rs:87-164

The json_strings_to_variant() function fails the entire batch on first invalid JSON:

let variant = VariantStruct::try_from_json(&json_str)
    .map_err(|e| DataFusionError::Execution(format!(
        "Failed to parse JSON at row {}: {}", i, e
    )))?;

Issue: No option for partial batch success or row-level error recovery

Recommendation: Consider adding a "skip invalid rows" mode with logging for production resilience.

---

Medium Priority

4. Variant-Specific Benchmarks Missing

Location: benches/core_benchmarks.rs

Current benchmarks don't measure Variant-specific operations:

Missing coverage:

• json_to_variant() conversion performance
• variant_to_json() serialization overhead
• Comparison: Variant columns vs. regular JSON columns
• Large JSON document handling (e.g., 10KB+ objects)

Recommendation: Add Variant-focused benchmarks to track regression in conversion paths.

5. Batch Concatenation Performance

Location: src/mem_buffer.rs:395

The query path concatenates multiple batches per time bucket without size checks:

let combined_batch = concat_batches(&schema, &batches)?;

Concern: High-cardinality scenarios with many small batches could cause GC pressure

Recommendation: Profile this path and consider batch consolidation threshold.

6. Limited WAL Recovery Granularity

Location: src/wal.rs:357-385

Recovery is binary - either skip all corruption or fail completely. No graduated recovery option.

Recommendation: Add configurable partial recovery (e.g., "skip up to N corrupt entries, then fail").

---

Low Priority (Documentation)

7. Performance Implications Not Documented

Location: docs/VARIANT_TYPE_SYSTEM.md

Missing sections:

• Performance overhead of Variant vs. native JSON types
• When to use Variant vs. regular columns
• Query optimization tips for Variant columns

8. Schema Evolution Limitations

Location: docs/VARIANT_TYPE_SYSTEM.md

Should document:

• Current limitations with schema changes
• Best practices for evolving Variant structures
• Migration strategies

---

🔒 Security Assessment

✅ No critical security issues found

Positive aspects:

1. JSON validation prevents injection attacks
2. 100MB batch size limit prevents OOM DoS
3. WAL version detection prevents format confusion
4. No SQL injection vectors (uses DataFusion's prepared execution)

Minor concern:

• No rate limiting on Variant conversions (could DoS with massive JSON payloads)
• Consider adding max JSON object size limit

---

📊 Test Coverage

Current coverage: Good integration test coverage through .slt files

Gaps identified:

1. No unit tests for Variant rewriters (see Issues #1 above)
2. No DELETE/UPDATE performance benchmarks
3. No memory pressure scenarios in benchmarks
4. Limited edge case coverage for Variant conversion

Recommendation: Add the missing unit tests before merge.

---

🎨 Code Quality Notes

Excellent patterns observed:

• Clean use of DataFusion's Transformed API in rewriters
• Proper logging with variant column counts (variant_insert_rewriter.rs:152-155)
• Safe WAL magic byte detection prevents format confusion
• Thread-safe concurrent access with DashMap

Minor style suggestions:

src/optimizers/variant_select_rewriter.rs:66-69

// Current (works but slightly unclear):
let (qualifier, field) = new_expr_with_alias.into_parts();
// ...
new_expr_with_alias = wrapped.alias_if_changed(alias);

// Suggested (more explicit):
let (qualifier, original_field) = new_expr_with_alias.into_parts();
let wrapped = create_udf("variant_to_json", vec![original_field]);
new_expr_with_alias = wrapped.alias(alias);

src/database.rs:103 - Silent skip could mask bugs:

// Current:
if idx >= columns.len() { continue; }

// Suggested: Add debug logging
if idx >= columns.len() {
    debug!("Column index {} exceeds batch length {}, skipping", idx, columns.len());
    continue;
}

---

📝 Summary & Recommendations

Before Merge:

• Add unit tests for VariantInsertRewriter and VariantSelectRewriter
• Document Parquet Variant version dependencies in code comments
• Add performance implications section to docs/VARIANT_TYPE_SYSTEM.md

Post-Merge (Future Work):

• Add Variant-specific performance benchmarks
• Implement schema evolution framework
• Add batch recovery mode for JSON conversion errors
• Profile batch concatenation performance
• Add troubleshooting guide for Variant type issues

---

✅ Conclusion

This PR represents high-quality work with:

• Solid architecture and implementation
• Comprehensive documentation
• Production-ready error handling and durability
• Performance-conscious design

The identified issues are minor and mostly centered around test coverage and documentation completeness. The core implementation is sound and follows DataFusion best practices.

Recommendation: Approve with the understanding that unit tests for the rewriters should be added before or shortly after merge.

Great work on the comprehensive documentation - the architecture docs will be invaluable for future contributors! 🎉

[claude]

Code Review - PR #16: Variant SELECT Rewriter and Architecture Documentation

Overall Assessment

Approval Status: ✅ Approve with minor suggestions

This is a high-quality PR with well-designed components. The VariantSelectRewriter and VariantInsertRewriter are clean, focused implementations that integrate well with DataFusion's analyzer pipeline.

---

Component Reviews

1. VariantSelectRewriter (src/optimizers/variant_select_rewriter.rs)

Quality: ✅ Excellent

What it does: Automatically wraps Variant columns with variant_to_json() in SELECT projections to ensure proper PostgreSQL wire protocol serialization.

Minor suggestion: Line 61 - Consider logging when get_type() fails:

expr.get_type(schema).map(|dt| is_variant_type(&dt)).unwrap_or(false)

The unwrap_or(false) safely defaults to not wrapping, but logging failures could help debug schema issues.

---

2. VariantInsertRewriter (src/optimizers/variant_insert_rewriter.rs)

Quality: ✅ Good overall

Potential Issues:

1. Name-based column matching (lines 52-57): Relies on exact column name matching between input and target schemas. Verify this works correctly with case-sensitive column names and when column order differs.

2. UTF-8 detection is conservative (lines 172-178): Only wraps literal strings and casts, not function calls returning strings. This is safe but might miss valid conversion opportunities. Acceptable design choice.

3. JSON validation timing: Invalid JSON only fails at execution time in convert_variant_columns(). This is late but safe and provides good error messages.

---

3. Database.rs Changes

Quality: ✅ Excellent

Highlights:

• Excellent error messages including row index and problematic values (lines 153-158)
• Proper row count preservation with with_row_count() (line 196)
• Comprehensive schema compatibility checks

Note: The fail-fast approach on invalid JSON is correct for ACID compliance, though one invalid row fails the entire batch. The error messages are detailed enough for users to fix issues.

---

4. DML.rs Changes

Quality: ✅ Excellent, with one verification needed

Security: ✅ Outstanding - Mandatory project_id filters properly enforce multi-tenancy

Verification needed: Lines 185-198, extract_project_id() uses val.to_string(). Verify that ScalarValue's Display implementation doesn't include quotes (e.g., ensure it produces project_123 not 'project_123').

Question: Lines 393-418 - The dual execution path runs operations on both buffered and committed data. If the same logical row exists in both, are updates counted twice? Please clarify the expected behavior.

---

Test Coverage Assessment

Overall: 🟡 Good but incomplete

Missing test cases:

1. ❌ Invalid JSON insertion error handling
2. ❌ NULL Variant values
3. ❌ Deeply nested JSON objects
4. ❌ Variant-specific integration test (INSERT/SELECT round-trip)
5. ❌ Very large JSON objects (performance/memory)

Recommendation: Add at least tests #1, #2, and #4 before merging.

---

Performance Considerations

Positive:

• ✅ Single-pass transformations
• ✅ Early bailout optimizations
• ✅ Batch processing

Considerations:

• JSON parsing on every INSERT has overhead (unavoidable)
• Large JSON batches could spike memory usage
• Recommendation: Document any batch size limits or consider streaming conversion for very large batches

---

Security Assessment

Status: ✅ Excellent - No security issues found

• ✅ No SQL injection vectors
• ✅ Multi-tenancy properly enforced
• ✅ No unsafe code
• ✅ Proper input validation

---

Rust Best Practices

Assessment: ✅ Excellent adherence

• Proper Result types throughout
• No unwrap() in production paths
• Strong typing and good separation of concerns
• Clear documentation

---

Action Items

Before Merge:

1. Add test for invalid JSON handling
2. Add test for NULL Variant values
3. Add Variant INSERT/SELECT round-trip integration test
4. Verify ScalarValue::to_string() behavior in project_id extraction
5. Clarify dual execution path behavior for UPDATE/DELETE row counting

Nice to Have:

• Add logging when type resolution fails (variant_select_rewriter.rs:61)
• Consider validating project_id is non-empty string, not just checking extraction success
• Document batch size recommendations for large JSON objects
• Add benchmark for Variant operations

---

Conclusion

The code is well-architected with proper error handling and security practices. The recommendations focus on test coverage and edge case handling rather than fixing bugs. Great work! 🎉

---

Review performed by Claude Code analyzing codebase patterns, Rust best practices, security, and performance implications.

[claude]

PR Review: Add Variant SELECT rewriter and comprehensive architecture docs

Overall this is a substantial, well-structured PR with meaningful improvements across the board. The unified storage model, session-aware DML, and timestamp-based bucket pruning are all solid additions. I've flagged several issues worth addressing before merging.

---

Bugs / Correctness Issues

1. SELECT * regression not fixed — test workaround hides the problem

In tests/integration_test.rs the test was changed from SELECT * to SELECT id, name, status_code, level with the comment "SELECT * fails due to Variant column encoding". This is a user-visible regression. The VariantSelectRewriter should handle wildcard expansion, or the cause should be investigated and fixed rather than worked around in the test.

2. buffer_consistency_test.rs passes wrong path to WALRUS_DATA_DIR

// tests/buffer_consistency_test.rs
unsafe { std::env::set_var("WALRUS_DATA_DIR", &cfg.core.timefusion_data_dir) };

BufferedWriteLayer::with_config now calls cfg.core.wal_dir() (which appends /wal), but the test sets the env var to the data dir root. This means walrus writes to ./data/test while the layer expects ./data/test/wal. The fix is to use cfg.core.wal_dir() — every other test site (buffered_write_layer.rs tests, main.rs) already does this correctly.

3. WAL deserialization: WAL_VERSION_IPC defined but never written

WAL_VERSION_IPC = 129 is declared and the deserialization path handles it, but serialize_record_batch still uses the v128 bincode path exclusively. The comment in deserialize_batch says "fall back to IPC (v129) for backward compat" but v129 is the newer version — the fallback order description is inverted. Either the IPC write path is missing (incomplete feature), or the backward-compat direction is wrong. This needs clarification.

4. UnsupportedVersion error message reports wrong expected version

// src/wal.rs
return Err(WalError::UnsupportedVersion {
    version: data[4],
    expected: WAL_VERSION_IPC,  // reports 129, but 128 is also valid
});

Both 128 and 129 are now valid. Reporting expected: 129 when encountering an unknown version (e.g. 130) is misleading. Consider changing the error to indicate a range or list of valid versions.

5. sanitize_query can panic on multi-byte UTF-8

// src/pgwire_handlers.rs
format!("{}...", &query[..MAX_LEN])

Slicing a &str at a byte offset (120) panics if the boundary falls inside a multi-byte character. Use query.char_indices().nth(MAX_LEN).map_or(query, |(i, _)| &query[..i]) or collect the first N chars.

---

Performance Concerns

6. concat_batches called while holding parking_lot::Mutex

In both query() and query_partitioned(), the code acquires the bucket mutex and then calls arrow::compute::concat_batches — which does a full data copy — while the lock is held. This blocks concurrent writers to the same bucket for the full duration of the copy. Consider doing the consolidation in the flush path or after releasing the lock by cloning the vec first.

7. resolve_table reloads storage configs from DB on every call

if let Some(ref pool) = self.config_pool
    && let Ok(new_configs) = Self::load_storage_configs(pool).await
{
    let mut configs = self.storage_configs.write().await;
    *configs = new_configs;
}

This issues a DB query and takes a write lock on every resolve_table invocation, including hot SELECT paths. Consider a TTL-based refresh (e.g. at most every N seconds) or a background refresh task.

8. wal_stats() does synchronous filesystem I/O in an async context

std::fs::read_dir is a blocking syscall, called from the async flush task. Wrap with tokio::task::spawn_blocking or use tokio::fs::read_dir.

9. Bloom filters enabled globally with fixed NDV

.set_bloom_filter_enabled(!self.config.parquet.timefusion_bloom_filter_disabled)
.set_bloom_filter_ndv(100_000)

Applying bloom filters with a fixed NDV of 100k to every column — including high-cardinality columns (UUIDs, full-text fields) and array/struct columns — will significantly increase Parquet file sizes without benefit for those column types. Consider restricting to known low-to-medium cardinality key columns.

---

Design / Architecture Concerns

10. Dual Variant→JSON conversion paths may interact unexpectedly

The PR introduces two separate mechanisms for converting Variant to JSON on SELECT:

• VariantSelectRewriter (logical plan analyzer) — wraps columns with variant_to_json() UDF
• VariantToJsonExec / variant_columns_to_json() (physical plan) — manually converts StructArray to StringArray

If both are active on the same query, a Variant column could be double-converted, potentially causing a runtime panic (the physical exec expects a StructArray but receives a String). Make the active path explicit: either always use the analyzer rewriter (and remove VariantToJsonExec), or always use the physical exec (and remove VariantSelectRewriter).

11. delta_session_from loses UDFs and catalog registrations

fn delta_session_from(session: &SessionState) -> Arc<dyn Session> {
    Arc::new(
        SessionStateBuilder::new()
            .with_config(session.config().clone())
            .with_runtime_env(session.runtime_env().clone())
            .with_default_features()
            .with_query_planner(deltalake::delta_datafusion::planner::DeltaPlanner::new())
            .build(),
    )
}

This builds a fresh session without the registered UDFs (JSON functions, Variant UDFs), catalog providers, or other extensions. If delta-rs internally plans the predicate using those functions, this will fail silently or produce incorrect DELETE/UPDATE results. Consider building this session once at startup, or finding a way to extend the existing session state with only the planner swapped.

12. extract_timestamp_range only handles top-level binary expressions

The filter pruning only recognizes flat BinaryExpr with Column("timestamp") on the left. Compound filters like AND(timestamp > x, timestamp < y) pushed as a single AND expression won't be decomposed, limiting pruning effectiveness. Consider recursively walking Expr::BinaryExpr with Operator::And.

---

Minor / Nits

• create_writer_properties type-string matching is fragile: Checking dt.starts_with("Timestamp") against a string field from YAML schema definitions may not reliably match all cases; matching against the Arrow DataType enum would be more robust.

• Batch size jump from 8192 → 65536: The 8x increase in datafusion.execution.batch_size is significant — good for throughput but increases per-query memory pressure. Worth benchmarking under memory-constrained conditions.

• d_data_dir default is ./data (relative path): The previous default was an absolute path (/var/lib/timefusion/wal). A relative default means data location depends on the process CWD, which is unpredictable in production. Consider keeping an absolute default like /var/lib/timefusion with the relative path only for local dev via .env.

---

Documentation

The three architecture docs (ARCHITECTURE.md, VARIANT_TYPE_SYSTEM.md, WAL.md) are detailed and useful. A few additions would help:

• Document that SELECT * on tables with Variant columns is currently limited (or fix it)
• Clarify the WAL version scheme (128 vs 129, and which write path produces which)
• In VARIANT_TYPE_SYSTEM.md, note that json_to_variant() is no longer required in INSERT statements — this is a user-visible behavior change (per the SLT test change) worth calling out explicitly

[claude]

PR Review: Add Variant SELECT rewriter and comprehensive architecture docs

Overall this is a solid PR with well-motivated changes — the unified storage model is a meaningful architectural improvement and the VariantSelectRewriter addresses a real need for proper PostgreSQL wire protocol serialization. The documentation additions are thorough. A few issues need attention before merging.

---

Critical

Incomplete analyzer rules list breaks SELECT * and other queries

src/database.rs (session config section) replaces DataFusion's full default analyzer rule set with only 4 rules:

let analyzer_rules = vec![
    ResolveGroupingFunction,
    VariantInsertRewriter,
    TypeCoercion,
    VariantSelectRewriter,
];

DataFusion's default set also includes InlineTableScan, CountWildcardRule, and ExpandWildcardRule. Dropping these breaks:

• SELECT * wildcard expansion (ExpandWildcardRule)
• COUNT(*) queries (CountWildcardRule)
• Inline table scans in CTEs/subqueries (InlineTableScan)

This directly explains the test regression in tests/integration_test.rs:

// Verify we can query specific columns (SELECT * fails due to Variant column encoding)

The failure is not a Variant encoding issue — it is the missing wildcard expansion rule. The fix is to insert the custom rules into the default list rather than replacing it entirely, e.g. by building around SessionStateBuilder::default_analyzer_rules().

---

Bugs

Version tracking for unified tables is always stale

In resolve_unified_table, the last-written version is looked up with an empty project ID:

versions.get(&("".to_string(), table_name.to_string())).cloned()

But when data is written, the version is recorded under the actual project ID (e.g. ("proj_abc", "otel_logs_and_spans")). These keys never match, so last_written_version is always None. The should_update arm (Some(_), None) => true means the table is refreshed on every read — correct in effect but for the wrong reason, and it will mask the real bug if the write path changes. The version should either be recorded under ("", table_name) on write, or resolve_unified_table should scan all versions for the given table name.

Wrong path set in buffer_consistency_test.rs

tests/buffer_consistency_test.rs sets:

unsafe { std::env::set_var("WALRUS_DATA_DIR", &cfg.core.timefusion_data_dir) };

But main.rs and buffered_write_layer.rs both correctly use cfg.core.wal_dir() (which is timefusion_data_dir.join("wal")). The test points to the data root instead of the WAL subdirectory.

WAL unsupported-version error message is misleading

src/wal.rs now accepts both v128 and v129, but the error case still reports expected: WAL_VERSION_IPC (129):

Err(WalError::UnsupportedVersion {
    version: data[4],
    expected: WAL_VERSION_IPC,  // always says "expected 129"
})

If a truly unknown version appears, the message incorrectly implies v129 was the only valid option. Consider a message string like "expected 128 or 129".

---

Design Concerns

In-place compaction during reads

MemBuffer::query and query_partitioned now compact multiple batches into one while holding the write lock:

let mut batches = bucket.batches.lock();
if batches.len() > 1 {
    if let Ok(single) = concat_batches(...) {
        batches.clear();
        batches.push(single);
    }
}

Read operations mutating shared state is surprising. A failed compaction (silently ignored via if let Ok) leaves data intact, but the row_count / memory_bytes atomics can diverge from the actual batch count during the compaction window. Consider moving compaction to the write path or to an explicit step, rather than as a side effect of reading.

Hardcoded timestamp column name in filter pushdown

extract_timestamp_range in mem_buffer.rs only prunes buckets when the filter references a column named exactly "timestamp":

let is_ts = matches!(left.as_ref(), Expr::Column(c) if c.name == "timestamp");

This silently skips pruning for tables where the timestamp column has a different name, and won't match qualified column references like table.timestamp. Wasting CPU on unneeded bucket scans is acceptable, but this should at least be documented so callers understand the limitation.

---

Performance Notes

8x batch size increase

datafusion.execution.batch_size and coalesce_target_batch_size were both raised from 8192 to 65536. This can significantly increase peak memory usage per query. Worth measuring in production before making this the default.

Bloom filters enabled by default

Bloom filters add write overhead and increase Parquet file size. The timefusion_bloom_filter_disabled flag is a good escape hatch, but this is a notable behavior change for existing deployments.

---

Minor

• wal_stats() counts all files in data_dir, including any non-WAL stray files that may exist there. A filename filter would make this more precise.
• delta_session_from rebuilds a session state with only with_default_features(), dropping all custom analyzer rules for DML operations. This is likely intentional (delta-rs needs its own planner), but a comment explaining why the custom variant rules do not need to run during UPDATE/DELETE would help future readers.
• Cargo.toml pins deltalake to an 8-character abbreviated commit hash (rev = "c4d506da"). Abbreviated hashes can be ambiguous; prefer the full 40-character SHA.
• In variant_select_rewriter.rs, the debug log counts all Expr::ScalarFunction nodes in the output, not just the newly-wrapped ones — the count will be inflated if the projection already contained scalar functions before the rewrite.
• MAX_LEN: usize = 120 is declared in sanitize_query (pgwire_handlers.rs) but appears unused based on the diff — confirm it is applied somewhere and not a dead constant.

[claude]

PR Review: Add Variant SELECT rewriter and comprehensive architecture docs

Overall this is a well-structured PR that closes an important gap — Variant columns now serialize correctly through the PostgreSQL wire protocol in both INSERT and SELECT paths. The documentation additions are thorough and the config consolidation is clean. A few areas worth addressing before merge are noted below.

---

Potential Bugs

1. is_utf8_expr only matches literals, not column references or parameter placeholders

In src/optimizers/variant_insert_rewriter.rs (line 169–175), is_utf8_expr only matches Expr::Literal and Expr::Cast. It will not match Expr::Column, Expr::Placeholder (parameterized queries via extended protocol), or other expression types that produce strings at runtime.

This means parameterized INSERTs like INSERT INTO t (variant_col) VALUES ($1) will not be rewritten at the logical plan level. The fallback VariantConversionExec at the physical layer handles the actual conversion, but the DataFusion type checker runs before it. Does is_schema_compatible_for_insert fully cover this gap for all parameter binding scenarios?

2. rewrite_input_for_variant only recurses into the first input child

In variant_insert_rewriter.rs (lines 91–101), the fallback arm only processes input.inputs().first() and then calls with_new_exprs(input.expressions(), vec![new_child]). For logical plan nodes with multiple inputs (e.g., a Join used in an INSERT INTO ... SELECT ... JOIN ... query), this replaces all inputs with just the rewritten first child, likely producing an incorrect plan. Should this iterate all inputs?

3. Potential byte-index mismatch in sanitize_query for non-ASCII queries

In src/pgwire_handlers.rs (lines 143–161):

let lower = query.to_lowercase();
let table_end = lower.find('(').or_else(|| lower.find("values")).unwrap_or(lower.len());
let table_part = query[..table_end].trim_end();

to_lowercase() can change byte lengths for some non-ASCII characters (Unicode case folding). The byte index from lower.find(...) is then used to slice into the original query, which can panic or produce garbled output when a multi-byte codepoint folds to a different-length sequence. Consider finding the index on the lowercased string and using that to slice lower rather than query, or find on the original string using a case-insensitive match.

---

Code Quality & Maintainability

4. Dual conversion paths for SELECT may cause confusion

The PR adds VariantSelectRewriter as an analyzer rule that injects variant_to_json() UDF calls into query plans. However, the existing VariantToJsonExec physical node (database.rs:228–290) also wraps every scan result that has Variant columns. In practice, after VariantSelectRewriter fires, the column is already Utf8 by the time VariantToJsonExec runs — making its downcast_ref::<StructArray>() a no-op for the common path.

This is defensible as belt-and-suspenders, but a comment explaining the intentional layering would prevent future maintainers from removing one layer thinking it is dead code.

5. Duplicate type compatibility logic

types_compatible / is_schema_compatible_for_insert in src/database.rs (lines 306–346) and schemas_compatible / types_compatible in src/mem_buffer.rs (lines 27–93) implement overlapping compatibility rules with subtly different semantics (the database version allows Variant-to-String in both directions; mem_buffer does not). As Variant support grows these will likely diverge. Consider consolidating in schema_loader.rs or a dedicated module.

6. DataSink::schema() vs TableProvider::schema() asymmetry

The DataSink impl for ProjectRoutingTable (line 2314) returns &self.schema (raw Variant types), while TableProvider::schema() (line 2389) returns create_insert_compatible_schema (Utf8View for Variant columns). The asymmetry is intentional but subtle enough to warrant a comment explaining why the DataSink deliberately returns the non-INSERT-compatible schema.

---

Performance

7. New ScalarUDF allocated per expression per query

In variant_select_rewriter.rs (line 35) and variant_insert_rewriter.rs (lines 106, 138), a fresh Arc<ScalarUDF> is allocated on every call to the rewrite functions. For high-QPS workloads this adds unnecessary allocation per query. Consider storing the UDF in the rule struct (constructed once) or using a OnceLock static.

---

Correctness

8. Schema partition change in otel_logs_and_spans.yaml is not backward-compatible

Adding project_id as the first partition column (before date) is a breaking change for existing deployed tables. Delta tables are immutable in their partition spec — existing files live under date=.../ paths, while new files will be written under project_id=.../date=.../. This means:

• Partition pruning on project_id will not apply to old files.
• The primary sort key changed (was timestamp, now level), affecting write-time file ordering.

Is there a migration strategy for existing deployments? If this targets greenfield deployments only, that should be documented clearly.

9. TIMEFUSION_FOYER_CACHE_DIR silently ignored after config rename

.env.test and .env.example drop TIMEFUSION_FOYER_CACHE_DIR in favor of deriving cache_dir() from TIMEFUSION_DATA_DIR. Any deployment that already sets TIMEFUSION_FOYER_CACHE_DIR will have it silently ignored after upgrade. Consider logging a warning on startup if the old env var is present.

---

Test Coverage

10. No unit tests for VariantSelectRewriter

src/optimizers/variant_select_rewriter.rs has no #[cfg(test)] module. Suggested cases:

• Variant column in SELECT is wrapped with variant_to_json()
• Aliased Variant column preserves the alias
• Column already wrapped with variant_to_json() is not double-wrapped
• Non-Variant columns are unchanged

The integration test delta (+10/-5 lines) is minimal for a feature touching the full INSERT/SELECT round-trip. A test exercising SELECT variant_col FROM table and asserting the result is a valid JSON string would strengthen confidence significantly.

---

Minor

• wal_stats() directory scope (src/wal.rs line 423): it scans self.data_dir which is now {TIMEFUSION_DATA_DIR}/wal. Verify the scan still covers the right files after the directory restructuring.
• AHasher in walrus_topic_key is non-deterministic by default (src/wal.rs line 245): AHasher uses a randomized seed per process. If the hash differs between restarts, previously-written WAL topics won't be found by key. Please confirm the seed is fixed/stable here, or document how the topic index file provides recovery coverage for this case.

[claude]

Code Review — PR #16: Variant SELECT Rewriter & Architecture Docs

This is a substantial, well-motivated PR. The core ideas (VariantSelectRewriter, WAL Arrow IPC migration, k-way merge recovery, unified table storage) are architecturally sound. I've organised feedback by severity.

---

🔴 Breaking Changes — Needs Documentation / Migration Path

WAL version bump (128 → 130) with no migration path

The comment in wal.rs says "wipe the WAL directory if upgrading" but this is only in a code comment. Any production operator upgrading will silently lose un-flushed WAL data unless they know to drain first. Consider:

• Adding a startup check that detects a version mismatch and errors loudly rather than silently skipping all old entries
• Or a one-shot migration tool

WAL sharding changes existing topic keys

Existing WAL topics were written to {hash} (no shard suffix). After this PR, recovery reads {hash}-00 through {hash}-03. A node with an existing WAL directory will recover zero entries from the old format. This is more dangerous than the version byte change alone.

Environment variable rename

WALRUS_DATA_DIR → TIMEFUSION_DATA_DIR, TIMEFUSION_FOYER_CACHE_DIR removed. Existing deployments will silently fall back to the ./data default (a relative path — see below). Should be in a migration notice.

Default data directory changed from absolute to relative path

// before
const_default!(d_wal_dir: PathBuf = "/var/lib/timefusion/wal");
// after
const_default!(d_data_dir: PathBuf = "./data");

./data is relative to the process's working directory. In containerised deployments this could be anywhere. The old absolute path was safer for production. Consider keeping an absolute default or at least documenting the change prominently.

---

🟠 Security

gRPC auth uses non-constant-time string comparison (grpc_handlers.rs)

Some(t) if t == expected => Ok(()),

== on &str can short-circuit on the first mismatched byte, leaking the length and prefix of the token via timing. For a static bearer token guarding an ingest endpoint, use subtle::ConstantTimeEq or constant_time_eq. The fix is one line but the vulnerability is real on a high-throughput network path.

---

🟡 Correctness / Behaviour

SELECT * still fails for Variant columns (acknowledged but not addressed)

The integration test was changed from:

// SELECT * from otel_logs_and_spans → 89 columns

to:

// Verify we can query specific columns (SELECT * fails due to Variant column encoding)
let row = client.query_one("SELECT id, name, status_code, level ...", ...)

This weakens regression coverage and leaves a known broken case. If VariantSelectRewriter properly wraps all Variant exprs at the root Projection, SELECT * should work. The fact that it doesn't suggests either the Projection node isn't being reached for star-expansions in the pgwire path, or there's a schema type mismatch after wrapping. This should either be fixed or filed as a follow-up issue with a root-cause comment.

VariantSelectRewriter doesn't peel Filter at root

wrap_root_projection peels Sort/Limit/Distinct/SubqueryAlias but stops at Filter. A query like:

SELECT payload FROM t WHERE payload->>'key' = 'val'

…which DataFusion might plan as Filter(Projection(...)), will not get the variant_to_json wrap. The current code comments this as "blocks wrapping" but that means the column comes back as raw binary on the wire, not a JSON string.

perform_delta_operation calls update_state() under write lock

let mut delta_table = table_lock.write().await;
delta_table.update_state().await...
let (new_table, rows_affected) = operation(delta_table.clone()).await?;

The write lock is held across an async network call (update_state likely hits S3). This blocks all concurrent readers/writers on that table for the duration. Consider acquiring state outside the lock or using a narrower critical section.

---

🟡 Performance

delta_session_from rebuilds a full SessionState on every DELETE/UPDATE

SessionStateBuilder::new().with_default_features() registers all built-in UDFs and aggregate functions. For DELETE/UPDATE heavy workloads this adds measurable overhead per DML call. Consider caching the session state (or at minimum the config) at Database construction time and reusing it.

TantivyConfig::is_table_indexed parses CSV on every call

pub fn is_table_indexed(&self, table: &str) -> bool {
    self.enabled() && self.indexed_tables().iter().any(|t| t == table)
}

indexed_tables() allocates a Vec<String> on each invocation. This is called on the hot query path. Cache the parsed set in a OnceLock<HashSet<String>> or pre-compute it at config load time.

WAL shard counter DashMap entries never cleaned up

shard_counter: DashMap<String, AtomicU64> grows one entry per distinct (project_id, table_name) topic and is never evicted. At scale with many tenants this is a slow memory leak. Either hook into table lifecycle events or use a bounded LRU.

---

🟢 Positive Changes Worth Calling Out

• WAL Arrow IPC format — replacing the hand-rolled CompactBatch + per-buffer bincode shuffle with Arrow's own IPC format is the right move. It correctly handles all Arrow types (List, Struct, Variant) and is self-describing, eliminating the schema registry lookup on recovery.

• k-way merge in for_each_entry — the BinaryHeap merge is a genuine improvement over materialising the entire post-cutoff slice into a Vec. The invariant comment ("at most one in-flight entry per shard alive at a time") is clear and the implementation correctly matches it.

• Configurable WAL fsync mode (ms / sync_each / none) — good operational knob, and the WalFsyncMode enum with a documented loss window for each mode is exactly what operators need.

• DML Variant type fix — the root cause (delta-rs's schema_force_view_types=true mismatching unshredded_variant() schema) is clearly explained, and the fix (injecting a custom SessionState with the flag overridden) is the correct scoped solution rather than a global config change.

• metrics.num_deleted_rows.unwrap_or(0) — prevents a potential panic when the Delta table returns None.

• Bucket-level timestamp pruning in MemBuffer::query — reduces unnecessary scan work for time-bounded queries. The bucket_overlaps_range logic is straightforward and correct.

• parking_lot::Mutex over std::sync::RwLock — for a write-heavy bucket structure this is the right call; parking_lot is non-poisoning and generally faster for short critical sections.

• timestamp_to_date_filter now handles all timestamp units — the old code only handled TimestampNanosecond, silently disabling date pruning for pgwire clients (which emit TimestampMicrosecond). The fix with checked_mul overflow protection is correct.

• Architecture docs — the three new docs (ARCHITECTURE.md, VARIANT_TYPE_SYSTEM.md, WAL.md) are genuinely useful. The data flow diagrams and key data structure descriptions will save future contributors significant ramp-up time.

---

Minor

• grpc_rejects_bad_payload test asserts ack.error.contains("decode") — the error from StreamReader::try_new on [0xde, 0xad] is implementation-defined. A less brittle check would be !ack.error.is_empty() or asserting status == Reject.
• The doc comment on convert_variant_columns (in the database.rs diff) appears to have multiple unrelated paragraphs run together — looks like a copy-paste merge artifact.
• d_data_dir: PathBuf = "./data" — the const_default! macro for PathBuf will call PathBuf::from("./data") which doesn't resolve at compile time; make sure this is clear to operators that it's relative to CWD at runtime, not the binary's location.

[claude]

PR Review: Add Variant SELECT rewriter and comprehensive architecture docs

Overview

This PR introduces a two-pass VariantSelectRewriter analyzer rule, unifies the Delta table storage model, integrates the buffered write layer into DML, and adds three comprehensive architecture documents. The core design is sound and the implementation quality is high.

---

Security

StorageConfig derives Debug with plaintext credentials (src/database.rs:309):

#[derive(Debug, Clone, Serialize, Deserialize, sqlx::FromRow)]
struct StorageConfig {
    s3_access_key_id:     String,
    s3_secret_access_key: String,  // ← exposed by {:#?}
    ...
}

If this struct is ever passed to a debug!(), error!(), or {:?} formatter, AWS credentials land in the log stream. The Serialize derive carries the same risk for any JSON serialization path. Recommend a custom Debug impl that redacts both key fields, and either remove Serialize or use #[serde(skip)] on the secret:

fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
    f.debug_struct("StorageConfig")
        .field("project_id", &self.project_id)
        .field("s3_access_key_id", &"[redacted]")
        .field("s3_secret_access_key", &"[redacted]")
        .finish()
}

---

Bugs / Correctness

HARD_LIMIT_MULTIPLIER is a divisor, not a multiplier (src/buffered_write_layer.rs:40):

const HARD_LIMIT_MULTIPLIER: usize = 5; // max_bytes + max_bytes/5 = 120%
// used as:
let hard_limit = max_bytes.saturating_add(max_bytes / HARD_LIMIT_MULTIPLIER);

The math is correct (20% headroom → 120% cap), but naming a divisor MULTIPLIER will confuse future readers. HARD_LIMIT_HEADROOM_DIVISOR or just inline the expression would be clearer.

should_refresh_table eagerly refreshes on every read when the buffer flusher has never written (src/database.rs:74):

(Some(_), None) | (None, Some(_)) => true,

The (Some(_), None) arm means: "table has a current version, but no last_written_version entry exists" → always refresh. On a read-heavy deployment that hasn't had a WAL flush yet, every query to a unified table triggers an update_state() S3 API call. The comment explains the bias is intentional to avoid a past bug, but it may be worth a follow-up to distinguish "never written" from "written but version not recorded."

peel() always wraps arms even when no Variant exists at the leaf (src/optimizers/variant_select_rewriter.rs:128-198):
Each peelable node (Sort/Limit/Distinct) is reconstructed inside new Arcs regardless of whether the inner Projection actually has Variant columns. wrap_projection has a fast path (if wrapped == 0 { return Ok(...) }) but the outer node is already rebuilt by then. Minor allocation cost, but consider peeking at the schema before rebuilding.

---

Known Gaps (documented but worth tracking)

1. INSERT INTO t SELECT col FROM staging silently skips Variant coercion (variant_insert_rewriter.rs:97-108): warns at warn! level but returns None, so the write proceeds with uncoerced types and fails at write time. A follow-up to return an explicit error (or support this path) would be less surprising than a deferred runtime failure.

2. SELECT variant_col FROM t UNION SELECT variant_col FROM t2 exits the wire as raw binary (variant_select_rewriter.rs:172-195): The warn! is the right short-term guard, but this silently corrupts pgwire output. Worth a GitHub issue so it doesn't get lost.

3. variant_functions.slt is disabled (tests/slt/variant_functions.README.md): The ->> operator's numeric/boolean → text coercion is untested via the SLT harness. The README correctly documents the gap, but the current PR adds the VariantSelectRewriter without re-enabling the tests that would catch coercion regressions.

---

Test Coverage

• No integration test exercises a Variant column INSERT → SELECT round-trip through pgwire. integration_test.rs:190-201 notes in a comment that it would work but the assertion uses only non-Variant columns. A single SELECT payload FROM otel_logs_and_spans WHERE ... assertion that parses the returned value as JSON would close this gap.
• Buffer consistency tests (buffer_consistency_test.rs) cover the MemBuffer/Delta read path well but don't include a Variant-typed column, so patch_table_scan's schema patching is only exercised end-to-end via manual testing.

---

Code Quality (minor)

• rewrite_values_for_variant and rewrite_projection_for_variant each instantiate JsonToVariantUdf::default() on every call. The UDF is cheap to construct, but hoisting it to a function parameter or a OnceLock would avoid repeated allocation under high INSERT throughput.
• build_optimize_session_state() in database.rs and delta_session_from() in dml.rs both independently set schema_force_view_types = false. A shared helper would prevent the two copies drifting.
• The O(n) → O(1) real_by_name HashMap in patch_table_scan (variant_select_rewriter.rs:89) is a good fix. Worth noting that it allocates a new HashMap per TableScan node in the plan; for queries with many table references this could be a hot path.

---

Strengths

• The two-pass design (schema patch first, then root wrap) is clean and the comment block at the top of variant_select_rewriter.rs explains the "why" exceptionally well.
• MAX_PEEL = 256 guard against adversarial plan depth is good defensive practice.
• DML rule ordering (VariantInsertRewriter before TypeCoercion, VariantSelectRewriter after) is correctly sequenced and documented at the registration site (database.rs:1023-1033).
• The normalize_timestamp_tz and cast_variant_columns_to_binary helpers have thorough error paths instead of panicking on downcast failures.
• Architecture docs are genuinely useful — the ASCII diagrams in ARCHITECTURE.md match the actual code flow.

---

Summary

The Variant SELECT rewriter is well-designed and the unified table model simplifies multi-tenancy. The main actionable items are: (1) redact StorageConfig credentials from Debug/Serialize, (2) add a pgwire round-trip test for a Variant column, and (3) rename HARD_LIMIT_MULTIPLIER to avoid the misleading name. The silent data-loss risks for INSERT … SELECT and Union queries are documented, but would benefit from follow-up issues to track them.

[claude]

Code Review — PR #16: Add Variant SELECT rewriter and comprehensive architecture docs

Scope note: 120 files, +30k / -2.6k lines. I've read the core modules (optimizers/, wal.rs, dml.rs, grpc_handlers.rs, buffered_write_layer.rs, plan_cache.rs, autotune.rs, mem_buffer.rs, database.rs, Cargo.toml) and the new test suite. The vendor patches and the complete tantivy tree received lighter scrutiny.

---

Overview

This PR delivers four interlocking features on top of a restructured storage model:

1. VariantSelectRewriter — two-pass analyzer rule that (a) patches TableScan schemas to expose real Variant types to downstream UDFs, then (b) wraps only the outermost projection with variant_to_json() for the wire.
2. Tantivy full-text search — transparent predicate rewriter that additively ANDs text_match() calls onto existing filters, with per-bucket in-memory indexes and a storage/service layer.
3. gRPC ingest path — bidi-streaming Write endpoint with constant-time bearer auth, memory backpressure, and per-batch Arrow IPC decoding.
4. Infrastructure — WAL sharding + FNV-stable keys, k-way merge recovery, WAL quarantine, plan cache, auto-tune, metrics, clock.rs abstraction, stats_table.rs.

The code is generally well-crafted and the inline documentation is excellent.

---

🚨 Critical / Correctness

1. Plan cache has no invalidation path

PlanCacheHook caches LogicalPlans keyed by canonical SQL text. A LogicalPlan embeds the table's SchemaRef at parse time. If a schema YAML is updated at runtime (new column added, type changed), all in-cache plans for that table are silently stale — queries will either error out at execution or, worse, silently drop new columns. There is no eviction hook wired to schema reloads.

The simplest fix is to include a schema-version token (e.g. a monotonic Arc<AtomicU64> bumped on each reload) in the cache key. Alternatively, flush the entire cache whenever schema_loader reloads.

// plan_cache.rs — no eviction on schema change visible in the code
static GLOBAL: std::sync::OnceLock<std::sync::Arc<PlanCacheHook>> = std::sync::OnceLock::new();

2. VariantSelectRewriter silently skips Filter-rooted plans

wrap_root_projection's peel only descends through Sort / Limit / Distinct / SubqueryAlias. DataFusion can produce plans rooted in a Filter when certain rewrite passes promote filters above the projection (e.g. with set datafusion.optimizer.filter_null_join_keys=true or custom optimizer rules). In that shape the peel hits the other => arm and emits a warn! but returns Variant binary unwrapped to the wire — the client receives undecodable bytes rather than JSON.

Adding LogicalPlan::Filter to the peel (descending into filter.input) would close the gap for the common case, or at least the warn! message should say "raw binary" more prominently to aid on-call debugging.

---

⚠️ Bugs / Edge Cases

3. Wrong error variant when WAL magic bytes mismatch

deserialize_wal_entry (wal.rs ~L567) reports WalError::UnsupportedVersion { version: data[0], expected: WAL_VERSION } when the 4-byte magic prefix doesn't match. But data[0] is 0x57 ('W') when magic is correct — if magic is wrong the first byte is arbitrary data, not a version number. The log message will say something like "Unsupported WAL version: 87 (expected 131)" which is actively misleading during incident triage.

A dedicated WalError::InvalidMagic { got: [u8; 4] } variant (or at minimum a distinct message branch) would make this immediately diagnosable.

4. read_entries_raw can silently succeed on a totally-corrupted shard

When every entry in a shard is corrupted, read_entries_raw returns ([], N) — i.e., empty results with a non-zero error count. The callsite in BufferedWriteLayer logs a warning but proceeds as if the shard has no entries. Depending on order-of-operations this can cause committed writes to disappear from MemBuffer after a crash. The quarantine mechanism handles individual entries but doesn't protect against a full-shard wipe. Consider propagating an error (or at least an error!() with explicit "DATA LOSS" language) when error_count == total_entries_attempted && total_entries_attempted > 0.

---

🛠️ Design Concerns

5. gRPC message size is unbounded

decode_and_insert in grpc_handlers.rs accepts any-length arrow_ipc payload. Tonic's default max incoming message size is 4 MB; a client that bypasses that (by setting max_decoding_message_size on the client) can send an arbitrarily large payload that the server will allocate without back-pressure until OOM. The WAL's 100 MB MAX_BATCH_SIZE limit is checked after the IPC decode, at which point the allocation has already happened.

Consider adding an explicit max_decoding_message_size to the IngestServer:

IngestServer::new(self)
    .max_decoding_message_size(256 * 1024 * 1024)

6. INSERT...SELECT with Utf8 source columns is a silent no-op

VariantInsertRewriter::is_utf8_expr matches only literal strings, not column references. INSERT INTO t (payload) SELECT col FROM staging where col is Utf8 skips the json_to_variant wrap and the write silently lands as a raw string instead of a Variant struct (or errors at write time, depending on how strict the Delta write path is). The warn! at line ~101 fires, but operators will see it only if they watch logs at variant_insert_rewriter=warn level.

This limitation is documented in the code, which is appreciated. Suggesting at minimum a test asserting the current behavior so it's explicit, and an integration-test that exercises the INSERT…SELECT path to confirm the error message is actionable.

7. MEMORY_OVERHEAD_MULTIPLIER tightened without a safety floor

The multiplier was cut from 1.5× to 1.15× based on a single benchmark run. estimate_batch_size() uses get_array_memory_size() which does not account for:

• Clone overhead during the transform_up rewriter passes (plan rewrites clone Arc-referenced batches)
• Tokio task stack allocations for concurrent flush tasks

If workloads with many nullable columns or large schemas hit a true 1.3× multiplier in production, the hard-limit headroom (20%) is consumed and the system starts rejecting writes earlier than configured. Consider adding a floor of 1.2× or exposing TIMEFUSION_MEMORY_OVERHEAD_MULTIPLIER as an env override so ops can tune it without a code change.

---

🔍 Minor / Style

8. Vendored Cargo.lock files

vendor/arrow-pg/Cargo.lock (4083 lines) and vendor/datafusion-postgres/Cargo.lock are committed. Crates vendored with [patch.crates-io] don't need their own lockfiles — the workspace root Cargo.lock is authoritative. These files will diverge silently over time and confuse cargo audit. Recommend removing them or adding them to .gitignore.

9. pick_shard wraps an AtomicU64 inside DashMap

let counter = self.shard_counter.entry(topic.to_string()).or_insert_with(|| AtomicU64::new(0));
counter.fetch_add(1, Ordering::Relaxed)

The AtomicU64 is always accessed under the DashMap shard lock (the RefMut guard is held during fetch_add), so the atomic operations are redundant synchronization. A plain DashMap<String, u64> with and_modify/or_insert would be cleaner, or alternatively use a DashMap<String, AtomicU64> accessed via get() (no lock held during the atomic op). Current code is correct but misleading.

10. walrus_topic_key stability test is good; add a migration note

The walrus_topic_key_is_stable test (wal.rs:L663) is excellent — it anchors the WAL key format. The comment that "wipe the WAL directory if upgrading" is critical operational knowledge. Recommend surfacing this in RUNBOOK.md (which is added in this PR) under a "WAL version upgrades" section.

---

✅ What's Done Well

• FNV-1a for WAL keys: switching from AHasher (per-build seed) to FnvHasher (deterministic) is exactly the right call for durable storage keys, and the regression test anchors it.
• k-way merge in for_each_entry: bounded recovery memory via the min-heap is a genuine improvement over materializing all entries into a Vec; the implementation is clean.
• Constant-time bearer auth (subtle::ConstantTimeEq): correct use of timing-safe comparison for token validation.
• Additive Tantivy rewriter: preserving the original predicate (for MemBuffer rows and post-flush Delta files that lack an index yet) is the right correctness invariant.
• Quarantine for corrupted WAL entries with 0600 permissions: practical observability without blocking recovery.
• Auto-tune with env_unset guard: user-set env vars always win; the "no overrides applied" log line makes the behavior auditable.
• MAX_PEEL guard in wrap_root_projection: belt-and-suspenders against adversarial plans is good defensive programming.

---

PR Size Observation

120 files changed / +31k lines is a challenging review surface. Several features here (gRPC ingest, Tantivy, autotune, metrics, plan cache, clock abstraction, stats table, benchmarks, docs) are independently useful and testable. Future PRs splitting these into logical units would make each easier to reason about, roll back, and bisect if something regresses in production.

[claude]

Code Review — PR #16: Variant SELECT Rewriter & Architecture Docs

This is a substantial PR (~31k additions). The documentation additions look thorough and accurate. Below is a focused review of the code changes.

Critical / Data Correctness Issues

variant_select_rewriter.rs lines 178-194 — Silent binary data for UNION/Aggregate/Join roots
The warn! fires but raw binary Variant bytes are returned over the pgwire wire. Clients will see garbage or crash. UNION ALL is a common pattern (e.g. pagination). Return a meaningful error or track this as a known GitHub issue.

variant_insert_rewriter.rs lines 48-57 — Index-based column matching can cross-correlate
variant_indices maps positional indices. If an INSERT specifies columns in a different order than the table schema (e.g. INSERT INTO t (b, a) VALUES (...)), the wrong columns get wrapped. Name-based lookup would be safer.

mem_buffer.rs lines 82-84 — Timezone mismatch treated as compatible
types_compatible returns true for timestamps with different timezones (UTC vs America/New_York). Merging such batches silently produces wrong results for absolute-timestamp queries. Force normalization via normalize_timestamp_tz before insert.

wal.rs lines 243-244 — project_id containing colon causes misrouted WAL entries
parse_topic uses split_once(':'). A project_id like urn:customer:123 splits incorrectly — table_name becomes customer:123. Validate project_id format at the API boundary, or use a different/encoded delimiter.

Security

database.rs — StorageConfig derives Serialize, exposing AWS credentials
StorageConfig has a good custom Debug that redacts secrets, but #[derive(Serialize)] will serialize them in plaintext if the struct ever reaches a log/metrics/response path. Wrap sensitive fields in Secret<String> or implement a redacting custom Serialize.

database.rs lines 503-509 — load_storage_configs reads all tenant credentials in one query
Any process with PG pool access can read every project's s3_access_key_id and s3_secret_access_key. High blast radius if PG credentials are leaked.

database.rs line 403 — build_storage_options debug log redaction misses AWS_ACCESS_KEY_ID
The filter excludes "secret" and "password" but not AWS_ACCESS_KEY_ID.

No input validation on project_id/table_name at trust boundaries
In dml.rs, extract_project_id takes the SQL literal value verbatim. A project_id containing : corrupts the WAL topic key (see above). Restricting to alphanumeric + - + _ at the API entry point would prevent this.

Correctness / Bugs

database.rs lines 1782-1787 — Error string matching for Delta conflict retry
error_str.contains("version") matches unrelated messages like "unsupported table version" or "missing table version", causing spurious retries. Use a typed delta-rs error discriminant instead.

database.rs line 557 — expect() panic on invalid AWS_S3_ENDPOINT
An invalid endpoint URL panics at startup rather than returning a startup error. Use map_err(...) and propagate.

dml.rs lines 591-593 — Write lock held across full Delta network operation
The write lock on table_lock is held while performing a potentially multi-second DELETE/UPDATE over the network. All concurrent reads and writes are blocked. Clone the table handle under the lock, release it, then perform the operation.

buffered_write_layer.rs lines 345-365 — WAL and MemBuffer insert are not atomic
If wal.append_batch succeeds but mem_buffer.insert fails (e.g. schema incompatibility), the WAL contains an entry that will be replayed on recovery. On recovery the schema-incompatible batch gets quarantined, losing data. A compensating WAL entry or rollback path is needed.

wal.rs lines 192-206 — persist_topic is not crash-safe
DashSet::insert and the file append are two separate non-atomic operations. A crash between them leaves the topics file missing an entry. for_each_entry calls list_topics() which reads the same file, so the orphaned shard is invisible on restart.

wal.rs lines 327-366 — read_entries_raw checkpoints past the cutoff
Entries with timestamp_micros < cutoff are deserialized then skipped, but checkpoint: true still consumes them. In partial-recovery scenarios old entries are permanently dropped rather than preserved.

plan_cache.rs lines 45-53 — Global OnceLock poisons test isolation
set_global called more than once silently fails. Tests that construct a fresh BufferedWriteLayer after the first will share the initial cache, causing test interference.

config.rs lines 431-435 — Silent fallback for unrecognized wal_fsync_mode
A typo in TIMEFUSION_WAL_FSYNC_MODE silently falls back to milliseconds mode, reducing durability. Add a warn! when the fallback triggers.

Performance

database.rs lines 900-925 — Pre-warm loop holds async read lock during network I/O
unified_tables.read().await.iter() holds the async read lock while calling extract_statistics (network I/O) in the loop body. This blocks all writers for the entire pre-warm cycle. Collect (name, table) pairs under the lock, release it, then iterate.

database.rs lines 68-76 — should_refresh_table acquires write lock on every read for tables never written by this process
The (Some(_), None) branch always acquires the write lock and calls update_state(). Under high concurrency with many multi-tenant tables this is a significant contention hotspot.

variant_select_rewriter.rs line 89 — HashMap rebuilt on every TableScan node
real_by_name is rebuilt from scratch for every TableScan during transform_up. For queries with joins or subqueries this is O(tables * fields) allocations per query.

Error Handling

variant_insert_rewriter.rs lines 100-109 — INSERT...SELECT silently passes through
warn! fires but Ok(None) returns the unmodified plan. The Utf8 → Variant mismatch is not caught at parse time and will either silently insert binary garbage or fail with a confusing type error at write time. At minimum raise DataFusionError::NotImplemented.

optimizers/mod.rs line 41 — timestamp_to_date_filter hardcodes "date" partition column
If a table uses a different partition column name (e.g. event_date), a filter on a non-existent column is produced silently.

Test Coverage Gaps

• VariantSelectRewriter: UNION ALL with Variant columns, Distinct::On with Variant output, MAX_PEEL=256 guard
• VariantInsertRewriter: INSERT with explicit out-of-order column list, INSERT...SELECT path (no-panic regression)
• mem_buffer.rs: schemas_compatible with timezone-mismatched timestamps, filter_batch_by_id_set with unexpected ID column type
• wal.rs: project_id containing :, persist_topic idempotency, read_entries_raw checkpoint behavior at cutoff boundary
• plan_cache.rs: LRU eviction, concurrent double-build, test isolation with global OnceLock
• config.rs: wal_fsync_mode with typo/fallback

Minor

• dml.rs line 440: has_committed is over-inclusive — returns true for any project that has written to the table, not just the current one. Causes unnecessary Delta scans for new tenants who have only written to MemBuffer.
• autotune.rs line 163: idempotency test passes trivially since serde default 4096MB already satisfies >= MIN_BUFFER_MB. The test does not verify autotune actually changed anything.
• buffered_write_layer.rs line 277: comment "prevents race condition" on try_reserve_memory is inaccurate — the CAS only protects reserved_bytes, not its sum with estimated_memory_bytes. Document this as a best-effort approximation.
• mem_buffer.rs: set_bucket_duration_micros is a process-global OnceLock side-effect. Two BufferedWriteLayer instances with different configs in the same process silently share the first bucket duration.

---

Overall this is a well-structured PR. The VariantSelectRewriter cleanly completes the round-trip type handling, and the documentation additions are genuinely useful. The most important issues to address before merge: timezone mismatch silent data corruption, WAL topic key corruption for colons in project_id, WAL-MemBuffer partial write atomicity, and StorageConfig serialization credential leak.

[claude]

Code Review — PR #16: Add Variant SELECT rewriter and comprehensive architecture docs

This is a large, ambitious PR that moves the system forward on several fronts simultaneously. Overall the code is well-structured and defensive, but the scope is significant and a few patterns warrant attention before merge.

Overview

Area	What changed
VariantSelectRewriter	Two-pass analyzer rule: patches TableScan schemas to restore real Variant types, then wraps the root projection with variant_to_json()
VariantInsertRewriter	Wraps json_to_variant() around Utf8 literals going into Variant columns at INSERT time
Tantivy sidecar	Full per-parquet-file text index subsystem (7 new source files + 5 test files)
gRPC ingest	Bidi-streaming Arrow IPC ingestion service
Plan cache	Cross-connection LRU for parameterized LogicalPlans
Auto-tune	Host-aware memory/disk/parallelism sizing
WAL rewrite	Arrow IPC format, FNV-1a sharding, version bump 130->131
DML	Buffered-layer integration, TOCTOU fix for concurrent UPDATE/DELETE
Docs	Three new architecture documents

---

Correctness Issues

1. VariantInsertRewriter: INSERT ... SELECT col FROM staging silently fails at write time (src/optimizers/variant_insert_rewriter.rs:101-108)

The rewrite_input_for_variant function only matches Values and Projection nodes. Any other shape (TableScan, Filter, etc.) returns Ok(None) after a warn!, so the INSERT proceeds without json_to_variant wrapping. The failure mode is surprising: the query succeeds at planning and only errors at write time with a type mismatch. Consider surfacing this as a planning error, or at minimum document what error message the user will see.

2. DmlContext::execute TOCTOU window between buffer and Delta checks (src/dml.rs:422-451)

let has_uncommitted = self.buffered_layer.is_some_and(|l| l.has_table(...));
// no lock held here
let has_committed = { ... read custom_tables and unified_tables ... };

Between the has_uncommitted check and the Delta operation, a concurrent flush could move data from MemBuffer to Delta. Rows in MemBuffer at check time could flush before mem_op runs AND before delta_op executes, leaving them undeleted. The comment documents "at worst we issue a Delta scan that matches nothing" but doesn't address this specific scenario where rows escape both passes.

3. extract_project_id uses val.to_string() which may include type decoration (src/dml.rs:203)

For ScalarValue::Utf8(Some("proj-123")), DataFusion's to_string() returns proj-123 (correct). For ScalarValue::Utf8View or future literal types, the format could differ. Consider explicitly matching only the string-typed scalar variants.

4. Plan cache schema-staleness assumption lacks a cross-reference (src/plan_cache.rs:22-29)

The module comment correctly notes: "If we ever add hot-reload of YAML schemas, this cache must also gain a schema-version token." This critical invariant lives only in one comment. Consider adding a cross-reference in schema_loader.rs so future hot-reload work doesn't miss it.

---

Security

5. StorageConfig derives Serialize which exposes credentials (src/database.rs:309-319)

The manual Debug impl redacts credentials, but Serialize does not. If a StorageConfig is ever serialized (future metrics endpoint, debug dump, observability tooling), AWS credentials would leak. The code's own comment flags this as "a future audit point." Recommend adding #[serde(skip)] on s3_access_key_id and s3_secret_access_key, or a custom Serialize impl that redacts them.

6. gRPC handler: no project-scoped authorization on project_id/table_name (src/grpc_handlers.rs:145-152)

An authenticated client can write to arbitrary project_id/table_name values. The bearer token provides authentication but not project-scoped authorization. If the intended model is "any valid token grants writes to any table," that should be documented explicitly; otherwise consider validating that the project_id matches what the token was issued for.

---

Performance

7. load_storage_configs logs INFO per-config entry (src/database.rs:513)

info!("Loaded config: {}/{}", config.project_id, config.table_name);

At scale (thousands of custom project tables), this floods startup logs. Recommend a single summary info!("Loaded {} storage configs", configs.len()) and demoting per-entry logging to debug!.

8. plan_cache.rs: statement.to_string() called before the cacheability check

let canonical = statement.to_string();
if !Self::cacheable(statement, &canonical) { return None; }

The AST is serialized on every extended-query Parse message, even for uncacheable statements. Testing the Statement variant (cheap) before calling to_string() (expensive AST serialization) would be more efficient.

---

Code Quality

9. WAL version bump is a breaking upgrade but RUNBOOK.md has no upgrade procedure

WAL_VERSION = 131 with the note "wipe the WAL directory if upgrading" is critical operational information. The new RUNBOOK.md is the right place for an explicit upgrade procedure: (a) stop service, (b) back up WAL directory, (c) wipe it, (d) restart. Currently the RUNBOOK doesn't mention this.

10. Union/Intersect/Except at SELECT root emits raw binary to the wire (src/optimizers/variant_select_rewriter.rs:186-203)

The warn! for unwrapped Variant columns at a Union/Aggregate root is good observability. However, "The pgwire client will receive undecodable bytes" describes silent data corruption. If the invariant holds that built-in schemas never produce Union at the wire root, a hard error on detection would be safer than a warning that could go unnoticed in production.

---

Test Coverage

The new test files are substantial. A few gaps worth noting:

• VariantSelectRewriter unit tests: SLT tests cover end-to-end behavior, but there are no unit tests for wrap_root_projection edge cases (Union/Aggregate at root, the MAX_PEEL guard firing).
• extract_dml_info unusual plan shapes: The other arm silently descends into the first child with a warn!. A regression here would silently lose predicate/project_id extraction.
• insert_coerce::rewrite_plan error path: The Err(e) fallback to the original plan is untested.

---

Summary

Must fix before merge:

• WAL upgrade procedure in RUNBOOK.md — breaking change with no documented recovery path
• StorageConfig::Serialize credential exposure — add #[serde(skip)] or custom impl on secret fields

Should fix:

• INSERT ... SELECT col for Variant columns should be a planning-time error, not a silent write-time failure
• Demote per-config info! in load_storage_configs to debug!
• Document or enforce the authorization model in the gRPC handler

Nice to have:

• Unit tests for wrap_root_projection edge cases (Union root, MAX_PEEL)
• Hard error (not warning) on raw-binary Variant reaching pgwire via Union/Aggregate root
• Cross-reference the plan-cache schema-staleness invariant in schema_loader.rs

The underlying approach is sound and the code shows careful attention to correctness: constant-time auth, redacted Debug for credentials, documented invariants, and consistent use of warn! for silent-failure paths. The issues above are about hardening edge cases and operational safety, not fundamental design problems.

[claude]

Code Review — PR #16: Add Variant SELECT rewriter and comprehensive architecture docs

Overall this is a well-designed PR with excellent inline documentation explaining the why behind design decisions. The core VariantSelectRewriter is sound. A few issues worth addressing before merge.

---

Bugs / Correctness

1. Timing side-channel in verify_bearer leaks token length (src/grpc_handlers.rs:178)

// The comment acknowledges this, but it IS a real issue:
// "Equal-length plaintexts compare in time independent of contents;
//  length mismatch short-circuits"
Some(t) if bool::from(t.as_bytes().ct_eq(expected.as_bytes())) => Ok(()),

subtle::ConstantTimeEq only provides constant-time comparison for equal-length inputs. If lengths differ, the comparison exits immediately, leaking the expected token's length. An attacker can binary-search the correct token length via timing. Mitigations:

• Use fixed-length tokens (e.g. 32-byte hex).
• Or pad/hash both sides before comparing: hmac::equal(expected.as_bytes(), got.as_bytes()) from the ring crate.
• The unit tests at the bottom of the file don't test this property.

2. is_variant_expr UDF name check is fragile (src/optimizers/variant_select_rewriter.rs:233)

if let Expr::ScalarFunction(sf) = expr && sf.func.name() == "variant_to_json" {
    return false;
}

This string-matching guard is a gotcha: if VariantToJsonUdf::name() ever changes, or if a different UDF with the same name is registered, this silently skips the idempotency check. Consider comparing against std::any::TypeId or matching by the UDF's type signature rather than its string name.

3. Possible double-wrap on re-analysis (src/optimizers/variant_select_rewriter.rs:212)

wrap_projection creates the variant_to_json UDF and wraps any Variant-typed expression. If DataFusion's analyzer runs VariantSelectRewriter more than once (which analyzers are permitted to do), the inner is_variant_expr guard at line 233 correctly short-circuits, but only if sf.func.name() matches. This ties correctness to the fragility mentioned in issue #2.

---

WAL Breaking Change (Operator Risk)

4. WAL version bump to 131 requires wiping the data directory (src/wal.rs:51)

// "Bumps: 131: Walrus collection key uses deterministic FNV-1a instead of AHasher
//  (AHasher's per-build seed silently stranded entries on upgrade).
//  wipe the WAL directory if upgrading."
const WAL_VERSION: u8 = 131;

The comment is there, but this is a sharp upgrade edge. Existing deployments that git pull and restart will lose any un-flushed WAL entries silently if they don't wipe first (the WAL entries from version 130 will be unreadable). Suggestions:

• Add a startup check: if the WAL directory contains any files that don't match WAL_MAGIC + WAL_VERSION, emit an error! or even panic! with explicit instructions before any recovery attempt.
• Document this in RUNBOOK.md (which was added in this PR).

---

Known Limitations (Documented, but Worth Tracking)

5. Variant wrapping skipped for INSERT … SELECT (src/optimizers/variant_insert_rewriter.rs:97)

// "Shapes like `INSERT … SELECT col FROM staging` … warn! so the limitation is visible"

The warn! is good, but this means any INSERT INTO t SELECT json_col FROM staging silently fails at write time. Consider rejecting this shape at planning time with a clear error rather than a runtime warn-then-fail.

6. Union/Intersect/Except at root produces raw Variant bytes on the wire (src/optimizers/variant_select_rewriter.rs:186)

The warn! is appropriate since this shape doesn't exist in any current schema, but it's worth filing as a tracked issue so it doesn't get forgotten.

---

Performance

7. MEMORY_OVERHEAD_MULTIPLIER reduced from 1.5 to 1.15 (src/buffered_write_layer.rs:38)

The measurement justification is thorough and credible. One note: the measurement was done at 4.7k inserts on a single project. If multi-project workloads or large Variant payloads materialize significantly more DashMap overhead, 1.15x might be tight. Recommend monitoring mem_estimated_bytes / actual_rss under production load before trusting this fully.

8. rewrite_input_for_variant allocates a new JsonToVariantUdf per DML statement (src/optimizers/variant_insert_rewriter.rs:113, 146)

Allocation is per statement (not per row), so this is fine. Just noting it's not a static — if allocation shows up in profiles for high-throughput INSERT workloads, hoisting this to a OnceLock would fix it.

---

Minor Code Quality

9. DmlContext::execute silently skips Delta when no committed data exists (src/dml.rs:440)

let has_committed = {
    let custom_tables = ...contains_key(&(self.project_id, self.table_name))
    || unified_tables.contains_key(self.table_name)  // ← no project_id filter
};

The comment explains this is intentional (unified tables are shared), but the asymmetry between custom (keyed by (project_id, table_name)) and unified (keyed by table_name only) is a subtle invariant. A test for "DELETE on project A doesn't touch project B's uncommitted buffer" would be valuable here.

10. Vendored datafusion-postgres partial checkout (vendor/datafusion-postgres/)

The vendor directory contains Cargo.toml and metadata files but no src/ directory, while Cargo.toml lists datafusion-postgres = "0.16" (from crates.io). This appears to be an incomplete vendor entry. If it's needed for cargo vendor reproducibility, it should be complete; otherwise, it should be removed to avoid confusion.

---

Positive Highlights

• The two-pass design in VariantSelectRewriter (schema patch → root wrap) is clean and the rationale is well-explained.
• StorageConfig credentials are correctly redacted in both Debug and serde::Serialize implementations.
• The quarantine_entry WAL corruption handling writes files with 0600 permissions — good security hygiene.
• patch_table_scan's O(n) HashMap lookup replacing O(n²) column iteration is a nice micro-optimization.
• The autotune.rs budget invariant is documented with fractions and reasoning — easy to audit.
• The insert_coerce.rs fix for multi-row INSERT placeholder type inference is non-obvious and the module-level comment explains it precisely.

---

CI

The test job runs cargo test --all-features which covers both unit and integration tests. The PR description mentions cargo test --test integration_test as a separate check item — this is already covered by --all-features, so the checklist may just need a wording update.

[claude]

Code Review — PR #16: Variant SELECT Rewriter + Architecture Docs

This is a substantial PR (+31 k / -2.7 k, 119 files). The overall approach is sound and the documentation is exceptionally good. Below are findings grouped by severity.

---

Overview

The core change is a two-pass AnalyzerRule (VariantSelectRewriter) that:

1. Patches TableScan schemas to restore real Variant types (replacing the "lying" Utf8View used to satisfy DataFusion's type-checker at INSERT time).
2. Wraps only the outermost Projection with variant_to_json() for wire serialization — keeping intermediate Variant UDFs operating on binary.

Accompanying additions: gRPC Arrow IPC ingestion, Tantivy full-text index, plan cache, host-aware auto-tuning, OpenTelemetry metrics, vendored crates.

---

Bugs / Correctness

1. insert_coerce.rs — expensive plan.clone() on every cache miss

pub fn rewrite_plan(plan: LogicalPlan) -> LogicalPlan {
    let result = plan.clone().transform_up(...);
    match result {
        Ok(p) => p,
        Err(e) => { warn!(...); plan }  // fallback to original
    }
}

The clone exists only to preserve plan for the error-path fallback. Since this runs on every cache miss (which includes all first-seen prepared statements), it pays a full plan-clone cost even when the rewrite succeeds and the clone is thrown away. Consider returning Result<LogicalPlan> from rewrite_plan and letting callers decide the fallback.

2. plan_cache.rs — has_placeholder can false-positive on SQL strings

fn has_placeholder(sql: &str) -> bool {
    sql.as_bytes().windows(2).any(|w| w[0] == b'$' && w[1].is_ascii_digit())
}

The comment correctly rules out bare contains('$'), but this byte-window check still matches $1 inside string literals (e.g. SELECT '$1 dollar'). That query would be incorrectly cached. This is a cache-pollution issue (not a correctness/security issue — DataFusion will still produce correct results), but it can cause unrelated parameterized statements to evict useful cached plans.

3. plan_cache.rs — silent OnceLock set failure

pub fn set_global(cache: std::sync::Arc<PlanCacheHook>) {
    let _ = GLOBAL.set(cache);  // silently drops if already set
}

If set_global is called twice (e.g. in tests, or after a future re-initialization path), the second call is a no-op and the new cache is silently discarded. At minimum this deserves a debug_assert! or warn!.

4. vendor/datafusion-postgres/ — source files absent

The PR adds metadata (Cargo.toml, .cargo-ok, etc.) for vendor/datafusion-postgres but no src/ files, while vendor/arrow-pg/src/ is fully present. If datafusion-postgres is patched via [patch.crates-io] pointing to the vendor directory, the missing source will cause a build failure for anyone doing a clean offline build. Please verify the vendor entry is either complete or confirm it's covered by a different mechanism (e.g. a git patch only).

---

Known Limitations Worth Calling Out in the PR Description

5. variant_insert_rewriter.rs — INSERT … SELECT not supported

other => {
    warn!(target: "variant_insert_rewriter", ...
        "INSERT input is not Values/Projection; json_to_variant wrapping is skipped …");
    Ok(None)
}

INSERT INTO t (payload) SELECT col FROM staging where col is Utf8 will silently fail at write time with a type-mismatch. The warn! is good, but this limitation isn't called out in the PR summary. Worth a follow-up issue number in the code comment.

6. variant_select_rewriter.rs — Union/Aggregate/Join root produces production warn!

warn!(
    "RAW BINARY VARIANT ON THE WIRE: peel() couldn't reach a Projection …"
);

If any production query shape legitimately reaches this branch (e.g. a UNION ALL across two variant-heavy tables), ops will see a loud alarming log on every query. Consider gating on a debug-level log unless the warning has fired fewer than N times per minute, or document exactly which query shapes trigger it so users can restructure.

---

Security

7. gRPC auth — correctly implemented ✓

verify_bearer SHA-256-hashes both sides before ct_eq, which eliminates the length-dependent early-exit in naive constant-time comparisons. The tests cover: wrong token, different-length token, missing token, correct token, and open-when-unconfigured. This is well done.

8. grpc_handlers.rs — grpc_token not logged ✓

The raw token is never written to a trace or log. Good.

---

Performance

9. variant_select_rewriter.rs — HashMap allocation per TableScan during planning

let real_by_name: HashMap<&str, &Arc<Field>> = real.fields().iter().map(|f| (f.name(), f)).collect();

This allocates a HashMap per TableScan node. For schemas with ≤ ~10 columns a linear scan over real.fields() would be faster (avoids hash computation + allocation). Planning isn't the hot path, but it's worth noting for schemas with many Variant columns and frequent short-lived connections.

---

Code Quality — Positives

• Two-pass design is clean — separating schema-patch from wire-wrapping avoids forcing intermediate operators to handle Utf8 and makes Variant UDFs compose naturally.
• Idempotency guard in is_variant_expr uses TypeId comparison rather than UDF name — robust against renaming.
• MAX_PEEL = 256 in wrap_root_projection prevents stack overflow on adversarial/deeply-nested plans.
• Auto-tuner correctly skips overrides when the user has set an env var, and the idempotency test is a nice sanity check.
• clock.rs frozen-clock UDFs are wired into SQL so test harnesses can drive time over a plain pgwire connection — elegant.
• Architecture docs are detailed and well-structured.

---

Test Coverage Gaps

• insert_coerce.rs has no dedicated unit test. A multi-row INSERT with mixed-type columns would be a good regression anchor.
• plan_cache.rs has no tests for the has_placeholder edge cases (dollar-in-string-literal, $$quoted$$, etc.).
• clock.rs test modifies global state (FROZEN_NOW) and calls unfreeze() at the end, but doesn't use a guard (e.g. scopeguard). A panic inside the test would leave the clock frozen for all subsequently-run tests in the same process.

---

Summary

Category	Count
Bugs / correctness	4
Known limitations needing tracking	2
Security (positive)	2
Performance	2
Test gaps	3

The Variant SELECT rewriting approach is sound. The most actionable items before merge are #1 (plan clone cost on every cache miss), #4 (missing vendor source files — may break offline builds), and #2 (plan cache pollution from false-positive placeholder check). The rest are lower severity or follow-up candidates.

[claude]

Code Review — PR #16: Add Variant SELECT rewriter and architecture docs

Overview

This PR introduces VariantSelectRewriter, a two-pass DataFusion AnalyzerRule that solves the Variant type serialization problem at the pgwire boundary. Pass 1 restores the real Variant type to TableScan projected schemas (the table provider lies with Utf8View so INSERT type-checking works). Pass 2 wraps Variant-typed expressions at the root projection with variant_to_json() so the pgwire client receives JSON text instead of raw binary structs. It also unifies UnifiedTables and CustomProjectTables storage models and adds comprehensive architecture documentation.

The overall approach is well-reasoned and the code is generally high quality. Several issues are worth addressing before merge.

---

Bugs

#[instrument] on the wrong function — wrong span name in traces (src/database.rs:1536–1557)

The #[instrument(name = "database.get_or_create_table", …)] attribute is placed between the two doc comments for list_file_uris, which means Rust applies it to list_file_uris, not get_or_create_table. Every call to list_file_uris will appear in traces as database.get_or_create_table, making production debugging misleading. get_or_create_table at line 1557 gets no span at all.

// current — broken
/// Legacy method for backward compatibility …
#[instrument(name = "database.get_or_create_table", skip(self), …)]
/// Return the live parquet file URIs …
pub async fn list_file_uris(&self, …)

pub async fn get_or_create_table(&self, …) { … }

// fix
/// Return the live parquet file URIs …
pub async fn list_file_uris(&self, …)

/// Legacy method for backward compatibility …
#[instrument(name = "database.get_or_create_table", skip(self), …)]
pub async fn get_or_create_table(&self, …) { … }

---

Known Gap — Binary Variant on the Wire for UNION / Aggregate

The warn! at variant_select_rewriter.rs:199–207 fires silently in production:

"RAW BINARY VARIANT ON THE WIRE: peel() couldn't reach a Projection through this
root node (Union/Intersect/Except/Aggregate/Join/etc.)…"

Any query like:

SELECT payload FROM t WHERE … UNION ALL SELECT payload FROM t WHERE …
SELECT payload, COUNT(*) FROM t WHERE … GROUP BY payload

will deliver undecodable binary bytes to the pgwire client. The fix (wrap each Union arm's leaf projection) is acknowledged in the comment. Since users of a Variant column are specifically the people most likely to write these queries, this silent data-corruption path should be either fixed or surfaced as a hard error (Err(DataFusionError::NotImplemented(…))) rather than a warning.

---

Design / Correctness

analyze() only skips LogicalPlan::Dml(_) — INSERT plans pass through

DataFusion's INSERT INTO … SELECT is represented as LogicalPlan::Dml with WriteOp::Insert, so it correctly gets skipped. However, if a future plan type bypasses this guard the Pass 1 schema patch would corrupt the INSERT path by introducing Variant types where the planner expects Utf8View. Consider matching exhaustively or adding an explicit comment that Dml covers all write ops.

has_custom_storage → TOCTOU in resolve_table (src/database.rs:1255–1263)

The lazy config reload happens at the top of resolve_table under a write lock, then has_custom_storage acquires a separate read lock. Between the two, another task could reload configs and flip whether a project is "custom". In practice storage configs change rarely and the worst outcome is resolving the wrong table once, but this is worth a comment.

should_refresh_table(Some(_), None) => true (src/database.rs:74)

The (Some(_), None) => true arm intentionally biases toward refresh when the process hasn't written. The comment notes this once caused a bug. This is a correct and well-documented defensive choice.

---

Performance

HashMap allocation on every patch_table_scan call

// variant_select_rewriter.rs:94
let real_by_name: std::collections::HashMap<&str, &Arc<Field>> = real.fields().iter()…collect();

patch_table_scan is called once per TableScan node per query via transform_up. For a schema with N fields, this allocates a HashMap every call. Because the fast-path at line 86 exits early when no Utf8View fields are projected, most queries avoid this. But for queries that do select Variant columns, consider whether real_by_name could be pre-built on ProjectRoutingTable and cached, since the real schema doesn't change.

---

Test Coverage

No end-to-end integration test selects a Variant column

tests/integration_test.rs:190 has a comment that VariantSelectRewriter is active but the test selects only id, name, status_code, level — none of which are Variant. There is no test that:

1. Inserts a row with a Variant column (e.g. attributes)
2. SELECTs that column via psql
3. Asserts the result is valid JSON text (not binary garbage)

This is the core contract of this PR and should be tested directly. The variant_functions.slt tests cover the UDFs in isolation but not the full pipeline through ProjectRoutingTable → VariantSelectRewriter → pgwire.

Suggested test addition in integration_test.rs:

// Insert with attributes Variant column, then select it back
let row = client.query_one(
    "SELECT attributes FROM otel_logs_and_spans WHERE project_id = $1 LIMIT 1",
    &[&"test_project"],
).await?;
let attrs: &str = row.get(0);
// Should be valid JSON, not binary
assert!(serde_json::from_str::<serde_json::Value>(attrs).is_ok());

No unit tests for wrap_root_projection

The peel logic has several branches (Sort → Projection, Limit → Sort → Projection, nested Distinct, Filter above Projection, MAX_PEEL guard). These branches are individually easy to get wrong and fast to unit-test with constructed LogicalPlan trees without a running server.

---

Minor / Style

• src/optimizers/variant_select_rewriter.rs:85: use datafusion::arrow::datatypes::DataType; is an inner use-import inside a function. Hoisting it to the top-level import block would be more consistent with the rest of the file.

• src/database.rs:657: // Convert &'static to Arc - it's fine since static lives forever — the comment is inaccurate; config::init_config() returns an owned value, not &'static. The clone is just a normal owned-value clone. No code change needed but the comment is misleading.

• src/database.rs:714–717: with_object_store_cache is kept as a no-op "for backward compatibility." If there are no known callers outside this repo, a #[deprecated] attribute would make this cleaner than a comment.

---

Documentation

The three new docs (ARCHITECTURE.md, VARIANT_TYPE_SYSTEM.md, WAL.md) are well-written, accurate, and include useful ASCII diagrams. Minor nit: VARIANT_TYPE_SYSTEM.md describes the -> and ->> operators but doesn't mention the MAX_PEEL / Union limitation that exists in the current implementation.

---

Summary

Area	Status
Core logic correctness	✅ Sound
#[instrument] on wrong function	❌ Bug — wrong span name in traces
UNION / Aggregate Variant wire corruption	⚠️ Known gap — warn-only silently corrupts data
End-to-end Variant SELECT integration test	⚠️ Missing
Unit tests for peel branches	⚠️ Missing
Performance (HashMap per call)	ℹ️ Low risk with fast-path, worth noting
Documentation	✅ High quality

The #[instrument] misplacement and the missing end-to-end Variant SELECT test are the most important items to address before merge.

[claude]

Code Review — PR #16: Variant SELECT Rewriter + Architecture Docs

Overview

This PR ships the second half of the Variant type system: a two-pass AnalyzerRule (VariantSelectRewriter) that (1) patches TableScan projected schemas to carry real Variant types instead of the Utf8View lie, and (2) wraps the outermost Projection's Variant-typed expressions with variant_to_json() for the pgwire wire. Supporting changes tighten the DML planner and unify table storage.

The code is high quality overall — well-commented, thoughtfully designed, with good unit tests for the new rule. A few specific concerns below.

---

Correctness concerns

1. Possible silent assignment drop in extract_assignments_from_projection (dml.rs:188)

Expr::Column(col) if col.name == *field_name => None,   // identity skip ✓
Expr::Alias(alias) if alias.name == *field_name => { … }
Expr::Column(_) => None,    // ← catches col.name != field_name — silent drop
_ => Some(…),

If DataFusion ever emits a bare Expr::Column("b") for UPDATE t SET a = b (i.e., not wrapped in an Alias), this arm silently drops the assignment instead of emitting ("a", Column("b")). Today DataFusion wraps all SET assignments in Alias, so this is probably safe — but it's a latent trap. Consider replacing with:

Expr::Column(col) if col.name == *field_name => None,  // identity
Expr::Column(col) => Some((field_name.clone(), Expr::Column(col.clone()))),  // rename

2. wrap_root_projection Filter peel silently changes HAVING semantics

peel peels through LogicalPlan::Filter under the justification that DataFusion sometimes promotes a filter above the outermost Projection. That is correct for pushdown rewrites. However, a HAVING clause compiles to a Filter node above an Aggregate, not above a Projection. If someone writes:

SELECT payload FROM logs WHERE project_id = 'x' HAVING payload IS NOT NULL

…and DataFusion places Filter(having) → Projection → TableScan, the peel would produce Filter → Projection(variant_to_json(payload)), which is correct. But the comment says "Some DataFusion rewrite passes promote a Filter" — it would be good to narrow the guard: only peel if the Filter references no columns that reference a Variant column (i.e., it's a pure predicate filter), or at minimum add an assertion/debug log when this branch is taken so surprises are visible.

---

Minor issues

3. DmlOperation::as_uppercase() is redundant (dml.rs:235)

strum::Display is already derived (emits "Update"/"Delete"); as_uppercase() returns "UPDATE"/"DELETE" and is used in one place. Either use strum(to_string = "UPDATE") on the variants, or drop as_uppercase() and call .to_string().to_uppercase() inline. Two parallel string representations of the same thing is a maintenance hazard.

4. patch_table_scan inlines use DataType deep in function body (variant_select_rewriter.rs:85)

use datafusion::arrow::datatypes::DataType;
if !lying_schema.fields().iter().any(|f| matches!(f.data_type(), DataType::Utf8View)) {

The use is file-local and buried inside the function. Move it to the top-level imports with the other DataType-related imports for readability.

5. convert_variant_columns silently skips out-of-bounds columns (database.rs:271)

if !is_variant_type(target_field.data_type()) || idx >= columns.len() {
    continue;
}

The idx >= columns.len() guard silently skips a Variant target column that has no corresponding batch column. This is likely unreachable (DataFusion schema enforcement upstream), but a silent skip on a Variant column means NULLs or garbage lands in Delta. Consider returning an error here instead of silently skipping.

---

Test coverage gaps

• Pass 1 (patch_table_scan) has no unit tests. It requires a real ProjectRoutingTable + DefaultTableSource, so it's harder to unit-test, but an integration test that actually selects a Variant column value via pgwire and verifies the returned JSON string would close this gap. The integration test comment at line 190 says "VariantSelectRewriter already serializes Variant columns", but the query selects only non-Variant columns (id, name, status_code, level).

• No test for the other => hard-error arm (Union/Aggregate/Join at root with Variant columns). A unit test building a Union of two variant_projection() plans and asserting analyze() returns Err(NotImplemented) would catch regressions if the error branch is accidentally softened.

• max_peel_short_circuits_on_pathological_depth — good test, works as intended. One nit: the assertion assert!(!is_variant_to_json_call(…)) implicitly documents that skipping wrapping is the chosen behavior on overflow. A comment explaining this is intentional (vs. a bug that should later be fixed) would help future readers.

---

Performance / design notes

• Arc::unwrap_or_clone in peel is the right call — avoids a clone when the Arc is exclusively owned (common in plan reconstruction).
• The real_by_name HashMap in patch_table_scan is O(n) per scan, fine for schema sizes.
• The MAX_PEEL = 256 guard is belt-and-suspenders given the parser's plan-depth limit. Documented clearly; no issue.
• The recursion in peel up to depth 256 uses the call stack. Rust's default stack is 8 MB; 256 frames of this function will be well under 1 MB, so no stack overflow risk in practice.

---

Documentation

docs/ARCHITECTURE.md, docs/VARIANT_TYPE_SYSTEM.md, and docs/WAL.md are thorough and accurate to the code I read. The ASCII diagrams especially help. One small note: the module listing in ARCHITECTURE.md omits tantivy_rewriter.rs from the optimizers/ section even though it exists in the tree — worth adding.

---

Summary

The core VariantSelectRewriter logic is sound. The idempotency guard (TypeId-based, not name-based) is a nice correctness detail. The main asks before merging:

1. Verify or fix the Expr::Column(_) => None arm in extract_assignments_from_projection — either confirm DataFusion always wraps in Alias, or make it explicit.
2. Add an integration test that selects a Variant column via pgwire and asserts it returns a JSON string.
3. Consider an error (not silent skip) for idx >= columns.len() in convert_variant_columns.

Items 2 & 3 are optional for this PR but strongly recommended as follow-ups.

[claude]

Code Review: Add Variant SELECT rewriter and comprehensive architecture docs

Overview

This is a large, well-motivated PR that introduces:

1. VariantSelectRewriter — a DataFusion analyzer rule that (a) patches TableScan schemas to use real Variant types instead of the Utf8View lie used for INSERT type-checking, and (b) wraps Variant-typed root projections with variant_to_json() for pgwire serialization
2. VariantInsertRewriter — wraps literal UTF-8 values going into Variant columns with json_to_variant()
3. DML refactor — a unified DmlContext abstraction for UPDATE/DELETE across both the in-memory buffer and Delta storage
4. WAL sharding — multi-shard WAL support with FNV-1a keying for stable cross-build keys
5. Unified table storage model — single Delta table per schema partitioned by [project_id, date]
6. Tantivy search integration, gRPC handler, metrics, plan cache, and extensive benchmarks

The design is sound and the comments/documentation are above average. Several specific items below warrant attention.

---

Issues

1. datafusion-variant pinned to branch, not a commit SHA (risk: silent breakage)

Cargo.toml:88

datafusion-variant = { git = "https://github.com/datafusion-contrib/datafusion-variant.git", branch = "main" }

Unlike deltalake (which is correctly pinned to a SHA), this is pinned to the rolling HEAD of main. Any push to that repo's main branch — including breaking API changes — will be silently pulled on the next cargo update. This is the same class of bug that the FNV-1a change (WAL v131) was added to prevent.

Fix: Pin to a specific commit SHA, matching the pattern already used for deltalake and datafusion-tracing.

---

2. INSERT … SELECT col FROM staging silently skips Variant wrapping

src/optimizers/variant_insert_rewriter.rs:97-108

other => {
    warn!(target: "variant_insert_rewriter", ...
          "INSERT input is not Values/Projection; json_to_variant wrapping is skipped");
    Ok(None)
}

The warning fires but the INSERT still proceeds — and likely fails at write time with a type-mismatch deep inside Delta. There is no test that exercises this path to confirm the failure mode is a clean error rather than silent data corruption or a panic. Since is_utf8_expr also intentionally skips Column references for the same reason, there are two related unhandled paths here.

Suggestion: Either return Err(...) with a clear user-facing message ("INSERT … SELECT into Variant columns is not yet supported"), or document a test that pins the actual failure behaviour.

---

3. Union/Intersect/Except with Variant at the root is a hard error, not a missing-feature warning

src/optimizers/variant_select_rewriter.rs:199-207

return Err(datafusion::error::DataFusionError::NotImplemented(format!(
    "Variant columns {:?} would exit the wire unwrapped at a {} root. ..."

SELECT payload FROM t1 UNION SELECT payload FROM t2 is a completely valid query that will now fail with a confusing NotImplemented. Given that the PR description says "today no built-in schema's wire-facing query shape produces these at the root", this error will appear as a regression from a user's perspective if they try union queries on tables with Variant columns.

Suggestion: Demote to a warn! and let the query proceed (producing raw bytes), which is at worst a degraded result rather than an outright failure. Alternatively, implement per-arm wrapping for Union as a follow-up and reference the issue.

---

4. Unnecessary clone in perform_update_with_buffer

src/dml.rs:445

let assignments_clone = assignments.clone();
// ...
.execute(
    |layer, pred| layer.update(project_id, table_name, pred, &assignments_clone),
    perform_delta_update(..., assignments, ...).instrument(update_span),
)

assignments is cloned into assignments_clone to pass into the mem closure, while the original moves into perform_delta_update. This is correct but the name (assignments_clone) is misleading — it looks like a debugging artifact. Consider renaming to mem_assignments/delta_assignments, or restructuring so the assignments.clone() is inline in the closure.

---

5. DmlOperation::as_uppercase duplicates strum-derived behaviour

src/dml.rs:235-240

fn as_uppercase(&self) -> &'static str {
    match self {
        Self::Update => "UPDATE",
        Self::Delete => "DELETE",
    }
}

DmlOperation already derives strum::Display and strum::AsRefStr. The as_uppercase method is never called from outside this file and returns the same string that Display would (strum uppercases enum variant names by default). This is dead weight that could diverge from the derived impl under a rename.

---

6. No end-to-end test for Variant column INSERT/SELECT — the primary feature

The unit tests in variant_select_rewriter.rs are good, but they work with EmptyRelation plans and never touch a real table or pgwire connection. The integration test (tests/integration_test.rs) inserts into otel_logs_and_spans but none of the asserted columns are Variant type.

The primary regression that VariantSelectRewriter exists to prevent — "SELECT payload returns raw binary bytes over pgwire" — has no integration test.

Suggestion: Add an integration test that inserts a row with a JSON payload into a Variant column and asserts the value returned by SELECT payload ... is valid JSON (not binary garbage).

---

7. shard_counter entries in WalManager are never removed

src/wal.rs:295-298

fn pick_shard(&self, topic: &str) -> usize {
    let counter = self.shard_counter.entry(topic.to_string()).or_insert_with(|| ...);
    ...
}

The shard_counter DashMap grows without bound: once an entry is inserted for a (project_id, table_name) topic, it is never removed, even after the table is dropped or the project is deleted. For deployments with many short-lived projects this could be a slow memory leak. The known_topics DashSet has the same issue but is at least bounded by the number of distinct topics ever written.

---

8. Test port selection range is too narrow

tests/integration_test.rs:46

let port = 5433 + rand::rng().random_range(1..100) as u16;

Only 99 possible ports. With #[serial] this is safe for sequential runs, but if CI ever runs multiple test binaries in parallel (e.g., cargo test --test integration_test and cargo test at the same time) port collisions will produce flaky tests. Consider random_range(1024..60000).

---

Positive Highlights

• Idempotency guard in is_variant_expr uses TypeId (not string name) — robust to UDF renames and duplicate registrations
• MAX_PEEL guard with a threaded test for pathological depth is excellent defensive coding
• WAL version stamp that refuses to start against mismatched on-disk data prevents the silent data-stranding that motivated the AHasher → FNV change
• FNV-1a key stability fix is well-motivated and the comment explaining why AHasher was wrong is valuable
• Credential redaction in Debug and Serialize for StorageConfig — correct and the #[serde(serialize_with)] pattern is clean
• K-way merge for WAL recovery preserves DELETE-after-INSERT ordering within a topic — correct and efficient
• DmlContext abstraction cleanly unifies the mem-buffer + Delta execution paths for UPDATE/DELETE

---

Summary

The core mechanics are solid. The two items I'd resolve before merge are #1 (pin datafusion-variant to a SHA to prevent silent API drift) and #6 (add an end-to-end pgwire test for Variant column round-trip). Item #3 (Union query regression) is worth fixing or explicitly tracking as a known limitation in a follow-up issue.

🤖 Generated with Claude Code

[claude]

Code Review — PR #16: Variant SELECT Rewriter + Architecture Docs

This is a very large PR (119 files, 31k+ additions). The review focuses on the highest-risk areas: the two new optimizer rules, WAL changes, and the Tantivy integration.

---

Overview

The PR delivers several distinct capabilities shipped together:

• VariantSelectRewriter – two-pass analyzer rule that restores real Variant types in TableScan schemas and wraps outermost SELECT projections with variant_to_json() for pgwire serialization
• VariantInsertRewriter – wraps Utf8 literals in json_to_variant() on INSERT
• Tantivy sidecar indexes – per-parquet-file full-text indexes with transparent text_match() UDF acceleration
• WAL improvements – version stamps, FNV-1a collection keys, per-topic sharding, quarantine for corrupted entries
• gRPC ingest handler, metrics, autotune, plan cache, and three architecture docs

The code is generally well-commented and structured. The doc PRD in module headers (especially variant_select_rewriter.rs) is a model worth following.

---

Issues Found

🔴 Critical / Potential Bugs

1. peel() recursion is a stack overflow risk in production (variant_select_rewriter.rs:130)

The test itself proves the problem:

// > MAX_PEEL nested SubqueryAlias should make peel() bail rather than
// recurse forever. DataFusion's own transform_up walk over a 300-deep
// plan blows the default 2 MiB test stack, so we run the whole thing
// on a larger thread — that itself is the assertion...
std::thread::Builder::new()
    .stack_size(16 * 1024 * 1024)
    ...

The test needs a 16 MiB thread because 300 deep aliases overflows 2 MiB. Tokio's default async task stack is typically 2 MiB. A query with ~200 nested CTEs/aliases could overflow in production before MAX_PEEL fires. Consider converting peel() to an iterative loop — the match arms are all linear (single child) so no true recursion is needed.

2. Hard error on Union/Aggregate root with Variant columns breaks valid queries (variant_select_rewriter.rs:196)

return Err(datafusion::error::DataFusionError::NotImplemented(format!(
    "Variant columns {:?} would exit the wire unwrapped at a {} root...",
    ...
)));

This converts a previous silent gap into a hard error. Any existing query using UNION ALL over a table with Variant columns will now fail with a NotImplemented error instead of returning data. The error message is excellent, but the behavioral change is a breaking regression for users. At minimum, gating this on a feature flag or falling back to warn! while emitting the raw bytes would be safer until branch-aware wrapping is implemented.

---

🟡 Significant Issues

3. VariantInsertRewriter has no unit tests

VariantSelectRewriter has a thorough peel_tests module covering idempotency, MAX_PEEL, and each peelable node type. VariantInsertRewriter has none. The is_utf8_expr → wrap_with_json_to_variant path and the Projection branch in rewrite_input_for_variant are completely uncovered at the unit level.

4. INSERT ... SELECT from a Utf8 column silently skips variant wrapping (variant_insert_rewriter.rs:96)

// Shapes like `INSERT … SELECT col FROM staging` (TableScan, Filter, etc.)
// don't currently get json_to_variant wrapping — the writer will hit a
// type-mismatch when staging.col is Utf8.
other => {
    warn!(...);
    Ok(None)
}

The warn! is good for visibility, but this silently produces a write-time type mismatch for the user. Since this is a known gap, a design note in the PR description would help reviewers decide whether to defer it.

5. contains_project_id and extract_project_id_from_expr are out of sync (optimizers/mod.rs:66 vs mod.rs:90)

The docstring says "keep them in sync", but extract_project_id_from_expr handles Expr::Not while contains_project_id does not. Divergence here means a query with WHERE NOT project_id = 'x' would return a project_id from the extractor but fail the has_project_id_filter guard, giving inconsistent routing/security-gate behavior.

6. Separator collision in walrus_topic_key (wal.rs)

":".hash(&mut hasher); // separator so ("ab","c") and ("a","bc") don't collide

This comment is wrong: with FNV's fold-into-hasher approach, hashing "ab", then ":", then "c" is equivalent to hashing the string "ab:c". If either project_id or table_name can contain a :, then ("a:b", "c") and ("a", "b:c") still collide. Since walrus keys are durable, a collision silently merges two independent streams. Recommend a length-prefix encoding: format!("{}\x00{}", project_id.len(), ...) or similar.

---

🟢 Minor / Suggestions

7. timestamp_to_date_filter widens > to >= silently (optimizers/mod.rs:36)

// Map timestamp comparisons to inclusive date bounds: a strict `timestamp > T`
// still admits rows on the same calendar day, so we widen `>` to `>=`...
let date_op = match op {
    Operator::Gt | Operator::GtEq => Operator::GtEq,

This is semantically correct (the original predicate still post-filters), but produces surprising EXPLAIN output where timestamp > '2024-01-01 23:59:59' generates a date filter date >= '2024-01-01'. A comment on the generated filter expression (or a debug! logging the rewrite) would make this easier to trace from query plans.

8. CI runs only cargo test --all-features, not the integration tests separately

The PR's test plan says:

• Run cargo test --test integration_test for integration tests

But .github/workflows/ci.yml runs only cargo test --all-features. The new tantivy integration tests (tantivy_e2e_test.rs, tantivy_transparent_test.rs) require MinIO — if they're gated by #[serial] or #[cfg(test)] conditionals and the CI MinIO service isn't properly initialized before those tests, they may silently skip. Confirm all new integration tests actually run (not just compile) in CI.

9. No benchmark regression detection

Five new benchmark files (benches/core_benchmarks.rs, etc.) are added but never run in CI. Without a baseline, future regressions in Variant/Tantivy paths will go undetected. Even a cargo bench --no-run check in CI would catch compile-time breakage.

10. MEMORY_OVERHEAD_MULTIPLIER reduced from 1.5× to 1.15× without rollout plan

The comment says this is empirically measured ("bench/multiplier_bench.py, 2026-05-17"). If production workloads with very wide schemas or different write patterns exceed the 1.15× assumption, the MemBuffer hard limit will be hit more aggressively. Consider documenting the measurement conditions so future on-call can evaluate whether the value is applicable.

11. MAX_BATCH_SIZE = 100MB WAL guard has no test

The WAL entry size guard (MAX_BATCH_SIZE: usize = 100 * 1024 * 1024) is mentioned in the source as preventing unbounded allocation from malicious/corrupted WAL, but there's no test that inserts a batch exceeding this limit and verifies the error path.

---

Strengths Worth Calling Out

• StorageConfig credential handling — The #[serde(serialize_with = "redact_str")] + custom Debug impl is a solid defense-in-depth pattern. Good model for future sensitive fields.
• Idempotency guard in is_variant_expr uses TypeId — Matching by concrete type rather than string name is more robust to renames or multiple registrations.
• WAL version stamps + FNV-1a migration — Detailed comments on why AHasher was broken and how FNV-1a fixes it. Excellent operational safety.
• Quarantine for corrupted WAL entries — 0600 file permissions on Unix is a good touch.
• Architecture docs — Genuinely useful. The data-flow diagrams in ARCHITECTURE.md will save future contributors significant ramp-up time.

---

Summary

The core feature (VariantSelectRewriter) is well-designed and well-tested. The main actionable items before merge are:

1. Convert peel() to an iterative loop (stack safety)
2. Either gate the Union+Variant hard error behind a flag, or document the breaking change explicitly in the PR description
3. Add unit tests for VariantInsertRewriter
4. Fix the contains_project_id / extract_project_id_from_expr NOT-divergence
5. Fix the walrus topic-key separator collision

Items 3–5 are low-effort relative to their correctness impact.

[claude]

Code Review — PR #16: Variant SELECT Rewriter + Architecture Docs

Scope note: The diff exceeds GitHub's 20K-line limit, so I reviewed the key changed files directly: variant_select_rewriter.rs, variant_insert_rewriter.rs, tantivy_rewriter.rs, database.rs, dml.rs, wal.rs, buffered_write_layer.rs, mem_buffer.rs, and the integration tests.

---

Overview

This PR lands three interrelated features:

1. VariantSelectRewriter — two-pass analyzer that (a) restores real Variant types in TableScan projected schemas and (b) wraps only the outermost Projection's Variant columns with variant_to_json() for the wire.
2. Unified table storage model — consolidates per-(project, table) routing into a cleaner UnifiedTables / CustomProjectTables split.
3. Tantivy transparent acceleration — additive text_match() rewrite so standard SQL predicates on indexed columns get a fast prefilter without changing correctness.

The architecture docs are a welcome addition and reflect the actual code accurately.

---

Strengths

VariantSelectRewriter design

• The TypeId-based idempotency guard (sf.func.inner().as_any().is::<VariantToJsonUdf>()) is correct and more robust than a string-name check; renaming the UDF won't silently double-wrap.
• MAX_PEEL = 256 with a warn! at the limit is a good belt-and-suspenders guard.
• The add_root_variant_projection fallback for Union/Aggregate/Join shapes ensures correctness even when the peel can't reach a Projection.
• Unit tests cover bare projection, peel through Sort/Limit/Distinct/Filter/Alias, double-analyze idempotency, and the pathological-depth case on a larger stack thread. This is thorough.

Security

• StorageConfig has a hand-written Debug impl that redacts credentials and uses #[serde(serialize_with = "redact_str")] — credentials won't leak through {:?} or JSON dumps.
• WAL quarantine files are written with 0o600 permissions — good practice for files that contain raw user data.
• extract_dml_info fails with a clear error if project_id is empty after full plan traversal, rather than silently routing to the wrong tenant.

Operational robustness

• WAL version stamp (check_wal_version_stamp) prevents silent format-mismatch data corruption on upgrade.
• The MEMORY_OVERHEAD_MULTIPLIER = 1.15 comment explicitly cites a benchmark date and measurement methodology — 1.5x → 1.15x is a meaningful correction, not a guess.
• Write lock held continuously across update_state → operation → snapshot swap in perform_delta_operation closes the TOCTOU window explicitly called out in the comment.
• should_refresh_table's (Some(_), None) => true case is correct; the comment traces the historical bug that motivated it.

---

Issues & Suggestions

Bug / Correctness

1. VariantInsertRewriter silent no-op for INSERT … SELECT from a table
rewrite_input_for_variant emits a warn! and returns Ok(None) for plan shapes that aren't Values or Projection (line 101–108). This means INSERT INTO t (payload) SELECT payload FROM staging where staging.payload is Utf8 will silently succeed at the SQL layer but fail at write time with a type mismatch. The warning may not be visible to the client. Consider surfacing this as a proper DataFusionError::Plan so the user gets actionable feedback rather than an opaque write-time error.

// current
other => {
    warn!(...);
    Ok(None)
}

// suggested
other => {
    return Err(DataFusionError::Plan(format!(
        "INSERT … SELECT from {} is not yet supported for Variant columns; \
         use INSERT … VALUES instead", other.display()
    )));
}

2. patch_table_scan fast path over-triggers
The fast path bails out if no Utf8View columns are found in the lying schema. But the check fires on any Utf8View column — including genuine non-Variant ones. This means the expensive HashMap-and-clone path runs any time a real Utf8View column exists alongside no Variant columns. The fix is to check whether any Utf8View column in the lying schema has a Variant counterpart in real_schema, but that requires two passes. Alternatively, keep the current conservative fast path but document it is an over-approximation.

Design

3. DmlContext::execute materialises delta_op: Fut eagerly

async fn execute<F, Fut>(self, mem_op: F, delta_op: Fut) -> Result<u64>

The future is built at the call site before execute checks has_committed. For perform_delta_update, that means perform_delta_update(...) (which calls perform_delta_operation — acquiring a write lock, calling update_state, etc.) is already constructed even if there is no committed data. In current Rust, constructing a Future is lazy — no work runs until it's .awaited — so this is safe. However, it's easy to accidentally break this assumption (e.g. if a future constructor does sync I/O). Making the signature delta_op: impl FnOnce() -> Fut would make the laziness explicit and avoid the footgun:

async fn execute<F, G, Fut>(self, mem_op: F, delta_op: G) -> Result<u64>
where
    F: FnOnce(&BufferedWriteLayer, Option<&Expr>) -> Result<u64>,
    G: FnOnce() -> Fut,
    Fut: std::future::Future<Output = Result<u64>>,

4. DmlOperation::as_uppercase vs strum::Display
DmlOperation derives strum::Display (which generates "Update" / "Delete") and also implements as_uppercase() (which returns "UPDATE" / "DELETE"). There are now two sources of truth for the operation name. Consider deriving strum::Display with #[strum(to_string = "UPDATE")] to unify them, or removing as_uppercase() if strum::Display can be adjusted to return uppercase.

Performance

5. Unnecessary clone in perform_update_with_buffer

let assignments_clone = assignments.clone();  // line 445

assignments is cloned unconditionally before we know whether the mem or delta path will be taken. Since assignments is Vec<(String, Expr)> and can be large for wide tables, cloning it when there is no buffered data is wasteful. Consider restructuring so the clone only happens on the mem path:

.execute(
    |layer, pred| layer.update(project_id, table_name, pred, &assignments),
    perform_delta_update(database, table_name, project_id, predicate, assignments, session).instrument(update_span),
)

(Moving the clone responsibility into BufferedWriteLayer::update if it doesn't need ownership.)

6. create_session_context builds real_by_name per TableScan per query
patch_table_scan rebuilds a HashMap<&str, &Arc<Field>> from routing.real_schema() on every TableScan visit. For a query that touches multiple partitions with the same routing table, this runs N times. Since real_schema() is stable for the lifetime of the routing table, this map could be pre-computed and cached on ProjectRoutingTable construction.

Minor / Style

7. should_refresh_table missing case comment
The match arm (Some(_), None) => true has a comment in the outer function but the (None, Some(_)) => true case has no inline comment. Adding one ("snapshot unversioned but someone else wrote version last") would make the match self-documenting.

8. Hardcoded Variant field names in convert_variant_columns

let fields = vec![
    Arc::new(Field::new("metadata", DataType::Binary, false)),
    Arc::new(Field::new("value", DataType::Binary, false)),
];

If delta_kernel's unshredded_variant() ever changes the field names, this will break silently at write time. Consider sourcing these names from a single constant shared with is_variant_type / variant_field in tests.

9. Test plan checkboxes are unchecked
The PR description includes:

- [ ] Run `cargo test` for unit tests
- [ ] Run `cargo test --test integration_test` for integration tests
- [ ] Verify Variant column SELECT returns JSON strings via psql

Please confirm these are passing before merge — especially the integration test, since this PR touches the query planner, DML, and flush paths which interact in subtle ways.

---

Security

• No SQL injection or command injection vectors observed.
• Credential redaction is correctly applied in both Debug and Serialize paths.
• Multi-tenant isolation via project_id filter enforcement is maintained for DML operations.
• WAL quarantine files are written with restricted permissions (0o600).
• The timefusion_projects DDL (in ensure_storage_configs_schema) uses parameterized queries via sqlx — no injection risk.

---

Summary

The core VariantSelectRewriter implementation is clean, correct, and well-tested. The WAL and memory-buffer improvements show careful benchmarking. The main actionable items are:

1. Turn the silent INSERT … SELECT warning into a user-visible error (bug, affects user experience).
2. Confirm the test plan is passing before merge.
3. Consider the DmlContext future-eagerness design (low risk now but a future footgun).
4. Minor: unify DmlOperation display strings, cache real_by_name on the routing table.

Good work overall — the design decisions are well-reasoned and the code is production-quality.

[claude]

Code Review — PR #16: Add Variant SELECT rewriter and comprehensive architecture docs

Overview

This is a large, well-structured PR that touches three primary areas:

1. VariantSelectRewriter — a two-pass DataFusion AnalyzerRule that patches TableScan schemas to expose real Variant types, then wraps the outermost projection with variant_to_json() for the wire
2. Database model refactoring — ProjectConfigs split into UnifiedTables + CustomProjectTables, per-bucket MemBuffer/Delta exclusion logic, timestamp timezone normalization, BinaryView→Binary cast at write time
3. WAL overhaul — Arrow IPC replaces the custom CompactBatch serializer, 4-shard-per-topic sharding, version-stamp compatibility guard, quarantine for corrupt entries

The code is well-commented and the architecture docs add real long-term value. The issues below are ordered by severity.

---

🔴 Critical

1. Core feature panics at execution time — the integration test is #[ignore]'d because of a known crash

tests/integration_test.rs contains test_variant_column_round_trips_as_json, the regression guard for the PR's stated purpose ("Verify Variant column SELECT returns JSON strings via psql"). It is marked:

#[ignore = "see TODO above — datafusion-variant requires extension marker on runtime Field"]

The TODO explains that variant_to_json panics with "Extension type name missing" at execution time. The patch_table_scan pass correctly injects the extension metadata into the logical plan schema, but that metadata is not surviving the trip into the physical executor's per-row Field. This means:

• The logical plan rewriting is correct
• Any live query SELECT <variant_column> FROM … will panic the server (not return an error — a panic in a tokio task will crash or abort)
• The PR's test plan item "Verify Variant column SELECT returns JSON strings via psql" cannot be checked off

This should block merge until the executor-side fix lands (either teach try_field_as_variant_array to recognise Struct{Binary,Binary} by shape without requiring the extension marker, or re-inject the marker at read time).

---

🟠 Important

2. WAL version went from 128 → 1 — backwards, with a silent data-loss upgrade path

// Before
const WAL_VERSION: u8 = 128;

// After
const WAL_VERSION: u8 = 1;

The new check_wal_version_stamp is correct in spirit: it refuses to open a directory stamped with a different version. But any operator upgrading from the previous build will have wal_version = 128 on disk and will hit:

refused to open WAL directory: version mismatch …

This is a hard startup failure requiring a manual wipe of the WAL directory (and losing all unflashed in-flight data). There is no RUNBOOK entry for this, no log message guiding operators to the required action, and no in-place migration. Consider:

• Adding an explicit RUNBOOK section for WAL version upgrades
• Emitting a human-readable error like "WAL version on disk is 128, binary expects 1. Delete {data_dir} to start fresh (unflused data will be lost)." rather than a generic version mismatch

3. unsafe { std::env::set_var } still present in concurrent tests without the removed ENV_MUTEX

The production code correctly removed the ENV_MUTEX + set_var pattern. But the three #[ignore]'d integration tests in database.rs still contain:

unsafe {
    std::env::set_var("AWS_S3_BUCKET", "timefusion-tests");
    std::env::set_var("TIMEFUSION_TABLE_PREFIX", …);
}

These tests are skipped in CI but the comment says "Run with cargo test -- --ignored locally". Since they run multi-threaded (tokio::test(flavor = "multi_thread")), calling set_var without protection is UB and can produce non-deterministic results. The #[ignore] doesn't make the unsafety acceptable. Consider using std::sync::Mutex or restructuring to pass config directly instead of going through env vars.

---

🟡 Minor

4. ProjectIdPushdown::contains_project_id and extract_project_id_from_expr can disagree

contains_project_id returns true for any project_id = <literal> regardless of the literal's type:

// optimizers/mod.rs — contains_project_id
matches!(
    (left.as_ref(), right.as_ref()),
    (Expr::Column(col), Expr::Literal(_, _)) | … if col.name == "project_id"
)

But extract_project_id_from_expr only matches Utf8 | Utf8View | LargeUtf8. So WHERE project_id = 42 passes has_project_id_filter but extract_project_id_from_expr returns None, and the DML extractor silently loses the project_id. At minimum, add a comment documenting this intentional gap (non-string equality is invalid and will error elsewhere), or unify the two helpers.

5. Delta exclusion OR-chain is unbounded

// database.rs
for (start, end) in &mem_ranges {
    delta_filters.push(Expr::BinaryExpr(BinaryExpr { … OR … }));
}

Each MemBuffer bucket appends one OR expression. With many small buckets (e.g., short bucket duration, high-frequency inserts), this can produce a deeply nested expression tree that slows DataFusion's predicate analysis. A defensive cap (log a warn and stop at N conditions) would protect against pathological cases.

6. Stale/concatenated doc comment on build_optimize_session_state

The doc block immediately above build_optimize_session_state (around line ~130 in the new database.rs) contains three different descriptions run together — one for the removed convert_variant_columns, one for normalize_timestamp_tz, and one for build_optimize_session_state. The function only has the last description's content. This appears to be a merge/paste artifact.

7. add_root_variant_projection computes variant_cols only for the debug log

// variant_select_rewriter.rs
let variant_cols: Vec<usize> = schema.fields().iter()
    .enumerate()
    .filter(|(_, f)| is_variant_type(f.data_type()))
    .map(|(i, _)| i)
    .collect();
if variant_cols.is_empty() {
    return Ok(plan);
}

variant_cols is computed eagerly (with allocation) and then only used in the debug! log. The is_empty() guard could be done with a lighter any() check, leaving variant_cols to be computed lazily or only when debug logging is enabled.

---

✅ Strengths

• The two-pass design (scan schema patch → root projection wrap) is well-reasoned and the module doc explains the why clearly
• Idempotency guard on double-analyze (TypeId check instead of name string) is exactly right
• MAX_PEEL = 256 guard against pathological depth is belt-and-suspenders done correctly
• Replacing CompactBatch with Arrow IPC is the right long-term direction — it handles all Arrow types and embeds the schema
• WAL quarantine for corrupt entries is excellent operational safety
• normalize_timestamp_tz is a pragmatic fix for the "+00:00" rejection in delta-rs — the comment clearly explains why
• Per-bucket MemBuffer/Delta exclusion replacing the single-cutoff approach is a correctness improvement with a clear explanation of the bug it fixes
• Unit tests in variant_select_rewriter.rs are thorough (peel paths, idempotency, MAX_PEEL, bare projection)
• Architecture docs are high quality and immediately useful

[claude]

Code Review — PR #16: Variant SELECT Rewriter + Architecture Docs

Overview

This PR adds a VariantSelectRewriter DataFusion analyzer rule that solves the Variant-wire-serialization problem cleanly: instead of converting at every scan boundary, it now wraps only the outermost Projection with variant_to_json(), letting intermediate operators (variant_get, jsonb_path_exists, etc.) work on binary Variant throughout. The unified table storage model (unified_tables + custom_project_tables) and DML buffered-layer integration round out the production-readiness changes. Documentation is thorough and well-organized.

---

Strengths

• Two-pass design is well-reasoned: Separating schema patching (pass 1) from JSON wrapping (pass 2) is the right call; the module-level doc makes the motivation clear.
• Idempotency guard uses TypeId instead of string name comparison — renaming the UDF won't silently break it.
• MAX_PEEL guard is sensible defensive programming; the warn log makes it traceable.
• DmlContext abstraction cleanly separates mem-buffer vs. committed-data paths without duplicating logic.
• Credential redaction: StorageConfig's manual Debug impl + #[serde(serialize_with = "redact_str")] on secret fields is proper defence-in-depth.
• Storage config lazy-reload with CAS TTL prevents thundering herd on the PG config table.

---

Issues

variant_select_rewriter.rs

[Minor] variant_cols Vec in add_root_variant_projection is built but only used for the early-return check and debug log count (line 226–227). The mapping loop independently re-checks is_variant_type. The Vec allocation is unnecessary — any() for the guard and count() from the mapping loop for the log would avoid it:

if !schema.fields().iter().any(|(_, f)| is_variant_type(f.data_type())) {
    return Ok(plan);
}

[Minor] transform_up calls recompute_schema() on every node (lines 71–76), not just the patched ones. For large query plans this is wasteful. The comment correctly explains why it's needed (stale DFSchema propagation after schema patching), but it may be worth revisiting once the approach stabilizes — e.g., returning Transformed::no for nodes whose inputs weren't actually changed, which would let DataFusion skip the recompute call.

[Nitpick] Test for max_peel_short_circuits_on_pathological_depth spawns a 16 MiB thread in a unit test (line 409–421). The comment explains why (DataFusion's own recursion overflows the default 2 MiB stack), which is fine. Just worth ensuring the CI runner has sufficient stack headroom, and that this test won't be silently skipped if spawn().join() itself panics for another reason without surfacing through the test harness properly.

[Moderate] No direct test for add_root_variant_projection: The branch that handles Union/Aggregate/Join/Window at the root (the other => arm in peel()) is not covered by the peel unit tests. Adding a test with an Aggregate plan wrapping a Variant projection would close this gap.

variant_insert_rewriter.rs

[Minor] No idempotency guard: Unlike VariantSelectRewriter, there is no check for an already-wrapped json_to_variant() call. If this analyzer runs twice, json_to_variant(json_to_variant('...')) would be emitted. DataFusion typically runs each analyzer rule once, but a guard matching by TypeId (same pattern as the SELECT rewriter) would be consistent and cheap:

fn is_utf8_expr(expr: &Expr) -> bool {
    // don't re-wrap an already-wrapped call
    if let Expr::ScalarFunction(sf) = expr
        && sf.func.inner().as_any().is::<JsonToVariantUdf>()
    {
        return false;
    }
    ...
}

[Observation] Error on unsupported INSERT shapes (line 101–106): The error message is clear, but other.display() could include literal values from the plan in logs. Acceptable for developer ergonomics, but worth keeping in mind if this surface ever processes untrusted data.

dml.rs

[Moderate] extract_dml_info silently drops predicates for multi-input nodes (lines 156–165): When an unhandled LogicalPlan variant is encountered, the function descends into other.inputs().first(). For multi-input plans (Joins, Unions), only the first child is walked, potentially losing predicates or project_id from the second child. The warn! log is appreciated, but a caller receiving an incomplete predicate with a non-empty project_id would proceed with a silent partial-operation. If Joins in DML are ever possible, this deserves stricter handling (return Err instead of silently descending).

[Minor] rows as i64 silent truncation (line 379): rows as i64 silently saturates/truncates for rows > i64::MAX. Practically impossible, but i64::try_from(rows).unwrap_or(i64::MAX) is more correct and equally readable.

[Minor] Dual lock acquisition in DmlContext::execute (lines 422–427): custom_tables and unified_tables are read-locked separately with an await between them. There's a theoretical TOCTOU window where a table is created between the two reads. The consequence is benign (a delta scan that matches nothing), but a single compound check or a brief note in the comment that the window is intentionally accepted would clarify intent.

[Nitpick] extract_assignments_from_projection filter logic (lines 183–196): The nested matches! condition for Alias filtering is correct but hard to follow at a glance. A brief inline comment explaining "filter out identity assignments where the aliased expression is the same-named column" would help future readers.

database.rs

[Minor] resolve_unified_table / resolve_custom_table code duplication (lines 1268–1330): Both follow the same pattern: check read-lock cache → check should_refresh_table → call update_table → return. The only difference is the key type (String vs (String, String)). A small generic helper would make future changes safer and reduce the drift risk. Not blocking, but worth a follow-up.

[Minor] extract_project_id on RecordBatch (lines 80–94): Falls back gracefully from Utf8View to Utf8 but silently returns None for other string representations (e.g., Dictionary<Int32, Utf8>). A warn! on the fallback-not-found case would make routing failures easier to diagnose.

[Observation] build_optimize_session_state (line 117–121): Creates a SessionState without TimeFusion's custom UDFs registered. If OptimizeBuilder evaluates expressions involving Variant UDFs (e.g., in partition pruning predicates), it would fail at runtime. This is almost certainly fine since Optimize only rewrites parquet files and doesn't evaluate application expressions — but worth a comment confirming this assumption.

---

Test Coverage Gaps

Gap	Severity
add_root_variant_projection (Union/Aggregate root) has no unit test	Moderate
VariantInsertRewriter double-analyze idempotency not tested	Minor
extract_dml_info with Window/Join/Union plan shape not tested	Minor
patch_table_scan with genuine (non-Variant) Utf8View columns not tested	Minor
Integration test port range (5433–5532, 100 ports) is narrow for parallel CI	Minor

---

Summary

The architecture is sound and the design choices are well-documented. The main actionable items before merge are:

1. Add a test for add_root_variant_projection covering an Aggregate or Union root.
2. Add an idempotency guard to VariantInsertRewriter (consistent with the SELECT rewriter).
3. Harden extract_dml_info against multi-input plan nodes — at minimum document the accepted limitation.
4. Widen the integration test port range to reduce the risk of flaky parallel CI failures.

Everything else is low-priority polish. The code is well-commented, the security considerations (credential redaction, tenant isolation guards) are handled correctly, and the tracing instrumentation is thorough.

[claude]

Code Review — PR #16: Add Variant SELECT rewriter and comprehensive architecture docs

Overview: This PR introduces a VariantSelectRewriter analyzer rule that wraps Variant-typed columns with variant_to_json() at the wire boundary, consolidates the DML layer into a DmlContext abstraction, adds a plan-level LRU cache, tantivy full-text indexing support, host-aware auto-tuning, gRPC ingest, and three architecture documents. The core design is sound, but at 119 files / 31K additions it is very difficult to review thoroughly in one pass — consider splitting into smaller PRs in future.

---

Correctness

wrap_root_projection peels through Filter (line 192–198, variant_select_rewriter.rs)
Peeling through a Filter that sits above the outermost Projection is risky. If the Filter predicate itself references a Variant column by name (not through ->>), patch_table_scan will have restored the Variant type on that column but the Filter's predicate Expr was already bound to the old stale DFSchema. In practice recompute_schema() in pass 1 propagates the corrected type bottom-up, so this should be safe — but the invariant is subtle and worth a comment explaining that pass 1 guarantees the Filter's schema is already correct before we peel into it.

has_committed check in DmlContext::execute uses table-only key for unified tables (dml.rs:423–426)
The comment correctly explains this: a Delta scan that matches nothing is the worst case. However, if the unified table never had any data (newly created table), we still issue a Delta UPDATE/DELETE against it, which may create an unnecessary Delta transaction. This is a performance nit, not a bug.

VariantInsertRewriter silently rejects INSERT … SELECT col FROM staging with a plan error (variant_insert_rewriter.rs:101–106)
The error message is actionable, so this is reasonable — but it means any client that uses INSERT INTO t SELECT * FROM staging (a common ETL pattern) will fail at parse time. This should be called out prominently in VARIANT_TYPE_SYSTEM.md and/or the schema YAML docs.

is_utf8_expr does not match Expr::Column (variant_insert_rewriter.rs:167–183)
Intentional per the comment, but the consequence is that pgwire INSERTs with column references in VALUES (unusual but possible) won't get wrapped. Fine for now, but the limitation should be tracked.

---

Security

AWS credentials stored as plaintext VARCHAR in timefusion_projects (database.rs:503–509)

s3_access_key_id VARCHAR(500) NOT NULL,
s3_secret_access_key VARCHAR(500) NOT NULL,

The StorageConfig struct redacts credentials in Debug and Serialize, which is good. However the credentials are stored at rest in PostgreSQL in cleartext. If the config DB is compromised, all custom-project S3 keys are exposed. Consider encrypting at rest (application-level AES-GCM wrap with a KMS-managed key) or using a secrets manager reference instead of the raw key. At minimum, the RUNBOOK.md should document the risk.

TIMEFUSION_ALLOW_INSECURE_AUTH accepts any password when set (pgwire_handlers.rs:56–63)
This is correctly warned about in the code, but the environment variable name (ALLOW_INSECURE_AUTH) could be confused with a less dangerous option. A more explicit name like TIMEFUSION_DEV_DISABLE_AUTH would make it harder to accidentally set in production.

WAL quarantine files use 0600 permissions — good. The write_owner_only function on Unix is correct.

---

Performance

transform_up with recompute_schema() on every node is O(plan_depth × schema_width) (variant_select_rewriter.rs:67–72)
For each node in a deep plan the full DFSchema is rebuilt. In practice DataFusion plans are shallow (< 20 nodes), but for queries with many subqueries or CTEs this could be noticeable. A future optimisation: skip recompute_schema() on nodes whose input schemas didn't actually change (i.e. Transformed::no from patch_table_scan).

patch_table_scan builds a HashMap every time a TableScan with a Utf8View column is encountered (variant_select_rewriter.rs:110)
This is O(real_schema_width) per scan. Fine for the current schema sizes, but worth noting for tables with hundreds of columns.

plan_cache.rs: PlanCacheHook::has_placeholder uses bytes().windows(2)
This is correct and fast. One edge case: a dollar-quoted string literal like $tag$... would not trigger a false positive because b'$' followed by a non-ASCII-digit character is skipped by the w[1].is_ascii_digit() guard. Good.

Schema-staleness comment in plan_cache.rs:25–30
The cache assumes schemas are immutable (compile-time include_dir!). This is a hard invariant that must be enforced as an operational constraint — if anyone adds runtime schema loading, the cache will silently serve stale plans. This is documented in the module comment, but it deserves a // INVARIANT: comment directly on the cache field or a debug_assert at initialization.

---

Code Quality

use datafusion::arrow::datatypes::DataType; inside a function body (variant_select_rewriter.rs:101)
Importing inside a function body is not idiomatic Rust. Move this to the top-level imports.

update_table has hardcoded MAX_RETRIES = 5 and jitter logic (database.rs:445)
This is the second retry/backoff loop in the codebase (the other is in buffered_write_layer.rs via MAX_CAS_RETRIES). Consider extracting a shared retry_with_backoff utility to avoid divergence.

perform_delta_update and perform_delta_delete both log info! on every call (dml.rs:493, 533)
In a high-throughput write path, info!-level logging fires for every UPDATE/DELETE. These should be debug! with a warn! only on error or slow paths. info! is appropriate for the first invocation or on large row counts.

DmlContext is a short-lived struct with no methods other than execute (dml.rs:392–435)
This is fine as an organizational pattern, but it could equally be a free function with the same parameters. The struct approach is arguably cleaner since it groups the context fields. Keep as-is.

variant_select_rewriter.rs: add_root_variant_projection collects variant_cols into a Vec<usize> solely to read its .len() for a log message (line 222)
The Vec allocation is avoidable — use schema.fields().iter().filter(…).count() for the log and schema.iter().any(…) for the early return.

---

Test Coverage

Good:

• peel_tests in variant_select_rewriter.rs cover: bare projection, Sort/Limit/Distinct/Alias/Filter peeling, idempotency (double-analyze), and the MAX_PEEL guard.
• Extensive tantivy test suite across multiple files.
• insert_coerce.rs handles the multi-row VALUES placeholder regression — a test specifically for this would prevent regressions (no dedicated test file for insert_coerce was added).

Missing:

• No test for VariantInsertRewriter with INSERT … SELECT col FROM staging (expected plan error).
• No test for convert_expr_to_delta stripping table qualifiers.
• autotune.rs has no tests — the RAM/disk fraction math is straightforward but platform-specific behavior (e.g. 0 total RAM on a container) is worth a unit test.
• plan_cache.rs: No test verifying that a schema-changing hot-reload (future concern) would be caught.

---

Structural / Process

• PR size: 119 files makes thorough review very hard. Future PRs of this scope would benefit from: (1) docs-only PR, (2) tantivy subsystem PR, (3) VariantSelectRewriter + DML PR.
• Vendored Cargo.lock files inside vendor/arrow-pg/ and vendor/datafusion-postgres/: these nested lock files are typically excluded from vendored crates and may confuse cargo vendor. Check whether they're needed.
• build.rs is new but not visible in the diff summary — ensure it doesn't introduce network calls or environment-dependent behavior that would break reproducible builds.

---

Summary

The core logic — two-pass VariantSelectRewriter, DmlContext abstraction, plan cache, WAL v2 format, and host-aware auto-tuning — is well-engineered and thoroughly commented. The primary concerns are: (1) plaintext credential storage in the config DB, (2) the ALLOW_INSECURE_AUTH footgun naming, (3) a handful of missing unit tests (multi-row INSERT coercion, insert … select error path), and (4) non-critical style issues (info! log spam on DML, inner use import). None of the above are blocking for the feature, but the security items should be tracked as follow-ups if not addressed before shipping to production.