Kafka Connect: Fix MongoDataConverter array conversion of timestamp and date types - #16604
Kafka Connect: Fix MongoDataConverter array conversion of timestamp and date types#16604wombatu-kun wants to merge 1 commit into
Conversation
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
not stale |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
b9aad0f to
30760af
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
…nd date types Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
30760af to
4d803c2
Compare
|
@laskoviymishka could you take a look when you have a moment? It's a 4-line type-conversion fix in the Mongo SMT, similar to #16606 that you merged, and the new tests fail without it. |
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice catch — this fixes a real runtime bug. The array path was calling asInt64()/asInt32() on DATE_TIME/TIMESTAMP values, so any array with a date or timestamp element threw BsonInvalidOperationException. Both lines now match the scalar path, and the 1000L * arithmetic on the timestamp branch is overflow-safe.
I don't think any of this blocks merge. The one thing I'd like to see is a nested-array test — the recursive path re-enters these same branches, and it's the only route to the fix the new tests don't exercise, so a [[BsonTimestamp(60, 1)], ...] case would lock it in.
The rest is optional: the two new tests are the only ones that cast the raw struct.get() instead of using AssertJ list coercion, and there's no DOCUMENT-encoding pair for the temporal types like the suite has for the other array cases.
Will wait for some time to this to settle and others to looks take a look.
|
|
||
| @Test | ||
| @SuppressWarnings("JavaUtilDate") | ||
| public void shouldConvertArrayOfTimestamps() { |
There was a problem hiding this comment.
The two new tests cover the flat ARRAY case, which is exactly the bug. The one path they don't touch is the recursive one — nested arrays re-enter this same method and hit the fixed DATE_TIME/TIMESTAMP branches again, and that's currently untested.
A cheap [[BsonTimestamp(60, 1)], [BsonTimestamp(120, 1)]] case would guard the re-entry so a future refactor can't silently break it. wdyt?
| } | ||
|
|
||
| // BsonTimestamp.getTime() returns the seconds component; the scalar path multiplies by 1000 | ||
| List<?> tsValues = (List<?>) struct.get("ts"); |
There was a problem hiding this comment.
These two are the only tests in the file that cast the raw struct.get() and assert element-by-element — everything else goes through assertThat(struct.toString()).isEqualTo(...). AssertJ can drop the cast and tighten it:
assertThat(struct.get("ts")).asList()
.containsExactly(new Date(60_000L), new Date(120_000L));Keeps the new tests consistent with the rest of the suite.
|
|
||
| @Test | ||
| @SuppressWarnings("JavaUtilDate") | ||
| public void shouldConvertArrayOfDateTimes() { |
There was a problem hiding this comment.
The suite already pairs ARRAY and DOCUMENT tests for the heterogeneous and empty-array cases; the temporal types only get the ARRAY side here.
DOCUMENT encoding routes through the scalar overload so it was never broken, but a mirror ...DateTimes/...Timestamps under ArrayEncoding.DOCUMENT would keep the pattern and catch a future regression on that path. Non-blocking — happy either way.
Closes #16603
Problem
MongoDataConverter(used byMongoDebeziumTransform) read BSON array elements with the wrong accessors whenarray.encoding=array(the default):DATE_TIMEelements were read withasInt64()andTIMESTAMPelements withasInt32(). Because those elements are actuallyBsonDateTime/BsonTimestamp,BsonValuethrowsBsonInvalidOperationException, so any MongoDB document containing an array of timestamps or date-times failed to convert. The scalar (non-array) paths already use the correct accessors.Solution
Read array
TIMESTAMPandDATE_TIMEelements with the same accessors as the scalar paths:asTimestamp().getTime()andasDateTime().getValue().This file lives under
org.debezium.connector.mongodb.transformsand was adapted from Debezium; the fix intentionally diverges from the (buggy) upstream snapshot.Tests
Added
TestMongoArrayConverter.shouldConvertArrayOfTimestampsandshouldConvertArrayOfDateTimes: they convert arrays ofBsonTimestamp/BsonDateTimewithArrayEncoding.ARRAYand assert the resultingjava.util.Datevalues. They fail before the change (withBsonInvalidOperationException) and pass after.AI Disclosure