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