Skip to content

parquet_encode: format timestamp columns as RFC3339Nano strings for compatiblity#4406

Closed
josephwoodward wants to merge 1 commit into
mainfrom
jw/mysqlcdc_timestampfix
Closed

parquet_encode: format timestamp columns as RFC3339Nano strings for compatiblity#4406
josephwoodward wants to merge 1 commit into
mainfrom
jw/mysqlcdc_timestampfix

Conversation

@josephwoodward
Copy link
Copy Markdown
Contributor

@josephwoodward josephwoodward commented May 7, 2026

No description provided.

Comment thread internal/impl/mysql/input_mysql_stream.go Outdated
@claude
Copy link
Copy Markdown

claude Bot commented May 7, 2026

Commits
LGTM

Review
One issue: the existing unit test for TYPE_DATETIME in schema_test.go was not updated to match the new RFC3339Nano string output, so task test:unit will fail.

  1. schema_test.go:132-137 — the "time.Time passthrough for datetime" case still expects time.Date(...), but mapMessageColumn now returns "2024-12-10T15:30:45Z". Test name and expected value need updating; consider adding coverage for sub-second precision and non-UTC zones.

@josephwoodward josephwoodward force-pushed the jw/mysqlcdc_timestampfix branch from b6c185f to 44601ab Compare May 7, 2026 10:20
@claude
Copy link
Copy Markdown

claude Bot commented May 7, 2026

Commits
LGTM

Review
Focused change converting MySQL CDC time.Time datetime/timestamp values to RFC3339Nano strings for downstream parquet compatibility, with the existing string-passthrough nil behavior preserved for zero/invalid timestamps. Tests cover sub-second precision and non-UTC zones.

LGTM

@josephwoodward josephwoodward force-pushed the jw/mysqlcdc_timestampfix branch from 44601ab to 447677d Compare May 7, 2026 11:52
@josephwoodward josephwoodward changed the title mysql_cdc: format timestamp columns as RFC3339Nano strings for parquet compatiblity parquet_encode: format timestamp columns as RFC3339Nano strings for compatiblity May 7, 2026
@claude
Copy link
Copy Markdown

claude Bot commented May 7, 2026

Commits
LGTM

Review
Clean refactor of encodingCoercionVisitor.visitLeaf that adds a time.Time case alongside the existing string case for TIMESTAMP encoding. The ts and unit-switch are correctly hoisted so both input types share the same conversion path, and the error message is updated to reflect the new accepted type.

LGTM

@josephwoodward josephwoodward deleted the jw/mysqlcdc_timestampfix branch May 7, 2026 11:55
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.

1 participant