Type system & inlined-data parsing (merge with upstream overlap)

[zfarrell]

Context

This is one ticket in a series carrying forward #12 foundation work. Read #12 first for repo context.

The fork expanded src/types.rs by ~1,000 LOC and added src/parse_values.rs to unify lenient (read-side) and strict (write-side) parsing of inlined values. Upstream has independently progressed (list/array support in PR datafusion-contrib#89, type alias normalization in datafusion-contrib#82, entity-name validation in datafusion-contrib#81) — so there is partial overlap here that requires careful merging, not blind porting.

Reference branch

ducklake-features/integration:

• src/types.rs (+1001 lines vs upstream)
• src/parse_values.rs — shared parser, ParseMode::Lenient vs Strict
• Tests: inline #[cfg(test)] in parse_values.rs; cross-coverage in tests/cross_engine_*.rs and write tests

Scope

1. Diff the fork's types.rs against the rebased base (post-Foundation: rebase integration onto upstream, drop pass-throughs, triage SLT failures #12). Bucket the changes into:
   • (a) functionality upstream now has independently — drop the fork's version
   • (b) functionality unique to the fork — port
   • (c) divergent implementations of the same idea — pick the better one (or merge), then drop the other
2. Port parse_values.rs as-is — it's a clean extraction with no upstream conflict.
3. Specific items confirmed by audit to exist in the fork but verify they're not already upstream:
   • VARCHAR(N) / CHAR(N) with explicit precision parsing
   • double precision synonym for DOUBLE
   • validate_no_trailing_after_paren check on parameterized types
   • Type promotion rules (is_type_promotion_allowed, used by ALTER TYPE in ALTER/DROP/CREATE schema evolution DDL #20)
   • Type normalization (case-folding) in is_type_promotion_allowed — R11-S-009 commit
   • NaN/Inf rejection in compaction delete_threshold range check — R11-S-010 (note: only relevant if compaction comes back later)
   • parse_decimal_string with checked arithmetic (R11-S-020)
   • i32::try_from for Date32 num_days to prevent truncation (R11-S-019)
   • checked_mul for timestamp unit conversions (R11-S-013)
   • saturating_add for null count accumulation (R11-S-014)
4. normalize_value limitation: the fork documented a known type-confusion limitation (R11-S-032) — preserve the doc comment so future contributors don't re-discover it.

Acceptance criteria

• cargo test passes including the parse_values inline tests and any cross-engine type round-trip tests
• Type promotion rules cover all DuckLake-spec-allowed promotions (cross-check the spec table)
• Decimal precision/scale round-trips through write → read without loss for at least: DECIMAL(38, 18), DECIMAL(10, 2), DECIMAL(1, 0)
• Date32, Timestamp(s/ms/us/ns), and Time roundtrip without overflow
• parse_values lenient mode degrades unknown types to UTF-8 (per fork's design); strict mode errors — both behaviors tested
• No duckdb crate imports

Dependencies

• Depends on: Foundation: rebase integration onto upstream, drop pass-throughs, triage SLT failures #12 (foundation)
• Related: ALTER/DROP/CREATE schema evolution DDL #20 (ALTER TYPE consumes is_type_promotion_allowed)

Out of scope

• Nested complex types (list, struct, map) beyond what upstream PR feat: support list/array column types in DuckLake type mapping datafusion-contrib/datafusion-ducklake#89 landed — fork did not complete this and it's a larger effort
• Geometry type support — fork treats as Binary (WKB), preserve that
• Decimal-formatter parity with DuckDB (the parity_basic_crud_after_insert issue) — that's covered in Foundation: rebase integration onto upstream, drop pass-throughs, triage SLT failures #12

Notes

• The branch's docs/edge-case-findings.md is worth reading for the type-system gotchas the agent team identified.
• The audit-flagged "documented limitation" in normalize_value is genuine and worth keeping — do not silently "fix" it without understanding why it's documented.

[zfarrell]

#23 Audit & Hardening: complete

Verified upstream-port/integration against the type system + inlined-data parsing work. Most R-fixes were already audited in #24; this ticket added missing round-trip coverage and confirmed no redundancy with upstream.

Phase 1: Upstream-overlap audit (src/types.rs)

Upstream PR	Status	Notes
datafusion-contrib#81 (entity-name validation)	fork carries equivalent	validate_name/validate_table_name in src/schema.rs:31,55; not duplicated in types.rs
datafusion-contrib#82 (type alias normalization + is_promotable)	fork carries equivalent in metadata_writer.rs	is_type_promotion_allowed (metadata_writer.rs:87) uses to_lowercase() for R11-S-009 case-folding; upstream's normalize_ducklake_type/is_promotable/types_compatible are NOT present and NOT duplicated. Single canonical implementation — fork's
datafusion-contrib#89 (list/array column types)	fork carries equivalent	parse_complex_type (types.rs:360) handles list<T>, array<T>, T[] notation. Reverse mapping in arrow_to_ducklake_type (types.rs:176). No duplicated logic

No redundancies found. No cleanups required. Foundation port already converged on a single implementation per area.

Phase 2: parse_values.rs intact

• Both ParseMode::Lenient and ParseMode::Strict variants present and exercised
• 8 inline tests pass: cargo test --features write-sqlite --lib parse_values → ok. 8 passed; 0 failed
• R11-S-019 verified: i32::try_from on Date32 num_days (parse_values.rs:130)
• R11-S-020 verified: parse_decimal_string has scale_u > 38 guard (parse_values.rs:365) + checked_mul/checked_add throughout (parse_values.rs:383, 403, 409)
• R11-S-013 verified: checked_mul on Timestamp nanosecond conversion (parse_values.rs:256)

Phase 3: Round-trip coverage added

tests/interop_type_tests.rs previously covered Date32, Timestamp(us), Decimal128(10,2). Added the gaps called out in #23:

• test_decimal_38_18_roundtrip — max precision Decimal128
• test_decimal_1_0_roundtrip — minimal precision Decimal128
• test_timestamp_second_roundtrip — TimeUnit::Second
• test_timestamp_millisecond_roundtrip — TimeUnit::Millisecond
• test_timestamp_nanosecond_roundtrip — TimeUnit::Nanosecond

All 5 new tests pass. Time(s/ms/us/ns) explicit roundtrip skipped because Arrow Time types are already tested via attack_type_roundtrip_time32 (adversarial_type_schema_tests.rs:597) and the type-mapping tests in src/types.rs (line 56–59 cover time_s, time_ms, time, time_ns variants).

Phase 4: normalize_value limitation preserved

tests/common/test_utils.rs:300-303 retains the R11-S-032 doc comment:

**Limitation**: This is type-unaware — it cannot distinguish between Float32 "1.0" and Decimal "1.00" since both normalize to "1.000000". For tests where numeric type precision matters (e.g., Decimal vs Float32 confusion), use assert_results_eq_strict instead.

No code change needed.

Phase 5: Type promotion rules spot-check

is_type_promotion_allowed (metadata_writer.rs:87) confirmed:

• All spec-permitted promotions covered: int{8,16,32}→int64 chain, uint{8,16,32}→uint64 chain, uint→larger-signed, float→double, timestamp→timestamptz
• Narrowings rejected: int64→int32, int32→int16, double→float (tested at metadata_writer.rs:827-829)
• R11-S-009 normalization: to_lowercase() applied to both source/target before match (metadata_writer.rs:88-89); test_type_promotion_case_insensitive covers VARCHAR/Varchar/FLOAT/Float
• Wired into ALTER TYPE: metadata_writer_validation.rs:286

No new tests added — existing coverage is solid (3 dedicated test functions + adversarial coverage pinning gap behaviors in adversarial_type_schema_tests.rs:1499-1594).

Acceptance criteria

• cargo test --features write-sqlite --lib parse_values passes (8/8)
• Type promotion rules verified for spec-allowed set
• Decimal precision: 38/18, 10/2, 1/0 round-trip without loss
• Date32 + all 4 timestamp precisions (s/ms/us/ns) round-trip
• parse_values lenient vs strict mode behavior tested (4 tests inline)
• No duckdb crate imports added
• No redundant overlap with upstream type code

Test count delta vs post-#22 baseline (~1,433)

• interop_type_tests: 5 → 10 tests (+5 net)
• All other suites unchanged
• Pre-existing failures (test_append_remove_column in write_tests) reproduce on upstream-port/integration unmodified — not introduced here

Regression suites — all green

• cargo test --features write-sqlite --lib: 415 passed
• cargo test --features write-sqlite --test interop_type_tests: 10 passed
• cargo test --features write-sqlite --test stats_tests: 7 passed
• cargo test --features write-sqlite --test delete_tests: 6 passed
• cargo test --features write-sqlite --test update_tests: 6 passed
• cargo test --features write-sqlite --test merge_tests: 5 passed
• cargo test --features write-sqlite --test row_id_tests: 6 ignored (pre-existing, Reconcile virtual_column_exec with upstream row_id (DESIGN required first) #22)
• cargo test --features write-sqlite --test adversarial_type_schema_tests: 94 passed

Branch

Pushed to zfarrell/feat/23-types: https://github.com/zfarrell/datafusion-ducklake/tree/feat/23-types

Commit: 7c06ad8 — test(types): decimal and timestamp precision round-trip coverage

Findings summary

No redundancies, no stale code, no overlaps with upstream PRs datafusion-contrib#81/datafusion-contrib#82/datafusion-contrib#89. The fork's types.rs and parse_values.rs are tight and the foundation port preserved every safety guarantee. Only delta needed was the missing precision round-trip coverage flagged in #23's acceptance criteria.