Skip to content

feat: support iso date for intervals - #123

Merged
MaxSiominDev merged 1 commit into
masterfrom
TNTP-10067-support-iso-date-for-intervals
Sep 9, 2026
Merged

feat: support iso date for intervals#123
MaxSiominDev merged 1 commit into
masterfrom
TNTP-10067-support-iso-date-for-intervals

Conversation

@MaxSiominDev

@MaxSiominDev MaxSiominDev commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Closes TNTP-10067

I haven't forgotten about:

  • Tests
  • Changelog
  • Documentation
    • JavaDoc was written
  • Commit messages comply with the guideline
  • Cleanup the code for review. See checklist

Related issues:

Copilot AI lite review requested due to automatic review settings September 7, 2026 06:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@MaxSiominDev
MaxSiominDev force-pushed the TNTP-10067-support-iso-date-for-intervals branch from 231f8c8 to 3ec3516 Compare September 7, 2026 11:11
@MaxSiominDev
MaxSiominDev requested a review from ArtDu September 7, 2026 13:54
@ArtDu

ArtDu commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Hi Max! Took a look at the code — it's done carefully, and I verified the test strings against the real io.debezium.time.Interval#toIsoString (debezium-core 3.2.7.Final): all 7 cases match exactly.

My question is not about the implementation, but about where this code should live.

As far as I understand, there is exactly one consumer here — our CDC, which needs intervals in the Debezium format. If that's the case, I'd suggest moving the converter there and closing this PR. Reasoning:

  1. The dependency points the wrong way. The SDK starts knowing about the Debezium format while knowing nothing about Debezium itself. The version we effectively pin to is recorded only in a test comment. If Debezium changes toIsoString, CDC will be the one to notice, but the fix and the release would have to happen in the SDK.

  2. This is a public contract for a single internal call site. parse/toIsoString in the SDK are forever — changing their behaviour later means a major version. In CDC the same converter is a private detail that can be fixed the same day.

  3. The format is not a general one. P-1Y-2M-3DT-4H-5M-6.789S is neither ISO 8601 (there the sign is single and leading) nor java.time: Duration.parse fails on such a string, I checked. It's Debezium's internal format, and it looks foreign in the SDK.

  4. In CDC some of the questions simply go away. For example, sec and nsec with different signs don't survive a round trip:

    new Interval().setSec(6).setNsec(-500_000_000).toIsoString()  ->  "P0Y0M0DT0H0M5.5S"
    Interval.parse("P0Y0M0DT0H0M5.5S")                            ->  sec=5, nsec=500000000
    

    This is not a parser bug — it follows from the model storing sec/nsec as independent signed fields while the string collapses them into a single number. In CDC it's enough to know which intervals actually come from Tarantool. In the SDK this has to be answered in the general case, with validation and documentation.

Nothing extra would be needed from the SDK for this: Interval already exposes all fields via getters and Adjust is public, so the converter can be written on top of the current API without a single change here.

The counterargument I can see: if we expect other consumers of a string format besides our CDC. Then keeping it in the SDK makes sense, but the format should probably not be Debezium-specific, and that's a separate task with a proper description (TNTP-10067 is currently empty, no acceptance criteria).

What do you think? If you agree, I suggest closing this PR and re-filing the task against CDC. If we do keep it in the SDK, I'll write up detailed code comments — besides the sec/nsec signs there are a couple more spots: Long.parseLong in applyField lets a NumberFormatException escape instead of an IllegalArgumentException on overflow (seconds are wrapped, the other fields are not), and the W output is an extension that Debezium neither emits nor understands — its signature has no weeks at all.

@MaxSiominDev
MaxSiominDev force-pushed the TNTP-10067-support-iso-date-for-intervals branch from 3ec3516 to 6758d8d Compare September 8, 2026 22:04
@MaxSiominDev

Copy link
Copy Markdown
Contributor Author

@ArtDu After a discussion with @bitgorbovsky in the TJ EE chat, we decided to add this feature without mentioning Debezium anywhere. I've already pushed new changes.

@MaxSiominDev
MaxSiominDev merged commit 4fdcf10 into master Sep 9, 2026
16 of 21 checks passed
@MaxSiominDev
MaxSiominDev deleted the TNTP-10067-support-iso-date-for-intervals branch September 9, 2026 12:43
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.

3 participants