Skip to content

Conversation

@amotl
Copy link
Member

@amotl amotl commented Dec 3, 2025

About

Mapping the TIME_NAIVE type didn't work before, because CrateDB can't store TIME types.

Review

Note this PR is stacked upon GH-112, but can be reviewed independently.

@coderabbitai
Copy link

coderabbitai bot commented Dec 3, 2025

Walkthrough

Adds support for Fivetran's NAIVE_TIME type with bidirectional type mappings and a new CrateDBKnowledge.replace_values() that parses TIME strings, normalizes them to a fixed date (1970-01-01), and converts to milliseconds since midnight. Adds python-dateutil dependency and updates tests/fixtures.

Changes

Cohort / File(s) Summary
Dependencies
pyproject.toml
Added python-dateutil<2.10 to support date/time parsing.
Type mapping & knowledge
src/cratedb_fivetran_destination/model.py
Added NAIVE_TIME mappings between Fivetran and CrateDB types and introduced CrateDBKnowledge.replace_values(request, record) to parse TIME values, normalize to 1970-01-01, and convert to milliseconds since midnight.
Data processing pipeline
src/cratedb_fivetran_destination/main.py
Imported CrateDBKnowledge and updated _files_to_records() to apply both FivetranKnowledge.replace_values() and CrateDBKnowledge.replace_values() during record normalization.
Changelog
CHANGES.md
Documented addition of NAIVE_TIME support.
Tests / Fixtures
tests/data/fivetran_canonical/input_fivetran.json, tests/data/cratedb_canonical/input_cratedb.json, tests/test_integration.py
Added time_val NAIVE_TIME entries to Fivetran fixtures and adjusted CrateDB fixture/test expected values to reflect the new time normalization behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review CrateDBKnowledge.replace_values() parsing and conversion logic for edge cases (midnight, 23:59:59.999, invalid formats).
  • Verify type mappings in TypeMap for correct bidirectional behavior and no duplicate/contradictory entries.
  • Confirm _files_to_records() application order and that value replacement is idempotent and consistent with tests.

Possibly related issues

  • #113 — Implements NAIVE_TIME mappings and CrateDB TIME transformation referenced by the issue; directly related to addressing NAIVE_TIME handling under history mode.

Suggested reviewers

  • seut
  • surister

Poem

🐰 I hopped through clocks and parsing light,

Fixed date set, the times ignite.
Strings became milliseconds clear,
NAIVE_TIME tamed, the data's here.
✨⏰

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and directly describes the main change: adding support for Fivetran's TIME_NAIVE data type, which is the primary focus of all modifications across the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed The pull request description clearly relates to the changeset, explaining the problem (CrateDB can't store TIME types) and the solution (adding support for Fivetran's TIME_NAIVE data type).
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch type-time

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as spam.

@amotl amotl merged commit 08ebf47 into sync-modes Dec 4, 2025
6 checks passed
@amotl amotl deleted the type-time branch December 4, 2025 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate error with NAIVE_TIME field when history_mode is enabled

3 participants