Visitar URL original
Floor the seconds when converting a pre-epoch date to a protobuf timestamp by PDGGK · Pull Request #39611 · apache/shardingsphere · GitHub
Skip to content

Floor the seconds when converting a pre-epoch date to a protobuf timestamp - #39611

Merged
terrymanu merged 2 commits into
apache:masterfrom
PDGGK:fix-cdc-pre-epoch-timestamp
Aug 30, 2026
Merged

Floor the seconds when converting a pre-epoch date to a protobuf timestamp#39611
terrymanu merged 2 commits into
apache:masterfrom
PDGGK:fix-cdc-pre-epoch-timestamp

Conversation

@PDGGK

@PDGGK PDGGK commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Changes proposed in this pull request:

  • Floor the seconds when converting a date to a protobuf timestamp, so pre-epoch values are not shifted forward by a second.

A protobuf Timestamp is a second plus a non-negative nanosecond offset into that second. converToProtobufTimestamp derives the seconds with integer division, which truncates towards zero, so below the epoch it names the second above the one the value is in:

if (timestamp instanceof Timestamp) {
    Timestamp value = (Timestamp) timestamp;
    return ...newBuilder().setSeconds(value.getTime() / 1000L).setNanos(value.getNanos()).build();
}
long millis = timestamp.getTime();
return ...newBuilder().setSeconds(millis / 1000L).setNanos((int) ((millis % 1000L) * 1000000L)).build();

java.sql.Timestamp#getNanos is already the non-negative offset into the second the value falls in, so pairing it with a truncated second does not describe the same instant:

input seconds nanos describes should be
new Timestamp(-1500) -1 500000000 -0.5 s -1.5 s
new Timestamp(-1) 0 999000000 +0.999 s -0.001 s

Off by a full second, silently, in a CDC stream.

The java.util.Date arm is worse — millis % 1000 keeps the sign, so it emits a negative nanos:

input seconds nanos
new Date(-1500) -1 -500000000

The protobuf Timestamp contract requires 0 <= nanos <= 999999999, so that message is invalid; Timestamps.checkValid rejects it and a consumer decoding it gets whatever its own library does with an out-of-range field.

Math.floorDiv and Math.floorMod fix both and are identical to / and % at or above the epoch, so nothing else moves.

Any replicated table holding a date before 1970-01-01 is affected — dates of birth, historical records, backfilled series.

Tests

assertConvertPreEpochTimestampToTimestampMessage runs -1500, -1, -1000, -86400000, 1500 and 0 through both arms and asserts, for each, that the nanos are inside [0, 1000000000) and that seconds * 1000 + nanos / 1000000 comes back as the original millisecond value.

The round trip is deliberate rather than restating the conversion formula — the two existing timestamp tests assert against the same expression the implementation uses, so they pass either way. On master the new test fails on the first value:

ColumnValueConvertUtilsTest.assertConvertPreEpochTimestampToTimestampMessage:170->assertRoundTrip:180
Timestamp -1500
Expected: is <-1500L>
     but: was <-500L>

mvn test -pl kernel/data-pipeline/scenario/cdc/core — 121 tests, all passing.

Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.
  • I have updated the Release Notes of the current development version. For more details, see Update Release Note

@PDGGK
Floor the seconds when converting a pre-epoch date to a protobuf time…
1cacf32 
…stamp

A protobuf timestamp is a second plus a non-negative nanosecond offset into
it. converToProtobufTimestamp derives the seconds with integer division, which
truncates towards zero, so below the epoch it names the second above the one
the value is in.

java.sql.Timestamp#getNanos is already the non-negative offset into the right
second, so new Timestamp(-1500) went out as -1s + 500ms, a full second later
than the value it came from. The java.util.Date arm emitted a negative nanos
instead, which the protobuf timestamp contract does not allow at all.

Math.floorDiv and Math.floorMod agree with / and % at or above the epoch.
terrymanu
terrymanu requested changes Aug 29, 2026
View reviewed changes

@terrymanu terrymanu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Result

Review Result: Not Mergeable

Feedback Mode: Change Request

Blocking Issues: 2

The timestamp arithmetic is correct, but the candidate still violates mandatory repository test and comment rules.

Blocking Issues

  1. The regression cases are combined into one test invocation

    • Evidence: ColumnValueConvertUtilsTest.java:167 invokes the public converter twelve times across different value types and epoch boundaries. The repository requires each test method to cover one scenario and invoke the target public method at most once; parameterized tests must use name = "{0}" (testing rules).
    • Impact: A failure in an early case prevents later Date and Timestamp boundaries from executing independently, and the test does not satisfy the repository’s focused-test contract.
    • Required Change: Convert the cases into parameterized invocations that call convertToProtobufMessage once per invocation, while retaining the nanos-range and round-trip assertions.

  2. The change introduces prohibited line comments

    • Evidence: Line comments were added in ColumnValueConvertUtils.java:148 and ColumnValueConvertUtilsTest.java:177. The project permits only Javadoc, TODO, and FIXME comments, including in tests (CODE_OF_CONDUCT.md).
    • Impact: Both changed files violate an explicit repository source rule even though Spotless and Checkstyle accept their formatting.
    • Required Change: Remove these comments and express any necessary intent through the code and test names.

Coverage

  • Candidate: Public PR #39611 at head 1cacf32a748bf50474c67149102f725c234f8842, based directly on 2e1eb7d8a1f1db529c2079138dcc8658636d8fcf. The final refresh confirmed the head and file list were unchanged.
  • Requirements: The PR description is the authoritative requirement; there is no linked issue or public review discussion.
  • Files accounted for: Both GitHub-listed files were reviewed. API patches matched complete parent/head file comparisons.
  • Behavior cluster: Pre-epoch Date and Timestamp conversion through ColumnValueConvertUtils into CDC Any values, socket delivery, and client conversion back through ProtobufAnyValueConverter.
  • Semantic assessment: Math.floorDiv and Math.floorMod correctly produce non-negative nanos for negative instants and preserve existing non-negative behavior, consistent with the protobuf Timestamp contract and java.sql.Timestamp.
  • Verification:
    • Focused ColumnValueConvertUtilsTest: exit 0; 26 tests passed.
    • CDC-core module suite: the sandboxed attempt exited 1 because Mockito could not attach its agent; the identical authorized rerun exited 0; 121 tests passed.
    • spotless:check checkstyle:check -Pcheck: exit 0; zero Checkstyle violations.
  • Review lenses: Root cause and behavior, blast radius and contracts, tests/runtime/operations, protocol compatibility, performance, documentation, dependencies, and style were assessed. The final convergence pass found no additional independent candidates.
  • Unresolved gaps: None that could change the blocker set.
  • CI scope: Code-correctness review only; GitHub CI was not reviewed.

@PDGGK
Address review: split the pre-epoch cases per scenario and drop line …
990bccb 
…comments

testing.md requires one scenario per test method with a single call to the
target, so the twelve invocations are now two parameterized tests with
name = "{0}", one per input type, each converting once.

CODE_OF_CONDUCT.md permits only Javadoc, TODO and FIXME comments; the line
comments in both files are gone.
@PDGGK

PDGGK commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Both fixed in 990bccb.

The twelve invocations are now two parameterized tests with name = "{0}", one per input type, each converting once: assertConvertPreEpochSqlTimestampToTimestampMessage and assertConvertPreEpochUtilDateToTimestampMessage, over six named cases. The nanos-range and round-trip assertions are kept.

The line comments in both files are removed, per CODE_OF_CONDUCT.md.

mvn test -pl kernel/data-pipeline/scenario/cdc/core - 132 tests, all passing. Checkstyle clean. Reverting the fix fails four of the parameterized cases.

terrymanu
terrymanu approved these changes Aug 30, 2026
View reviewed changes

@terrymanu terrymanu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Result

Review Result: Mergeable

The latest head correctly repairs pre-epoch timestamp conversion and resolves both previous review blockers. No current-head code-correctness blocker remains.

Evidence

  • ColumnValueConvertUtils.java:147 now uses floorDiv for seconds and floorMod for millisecond fractions. With a positive divisor, this produces nanos in 0..999,000,000 while preserving the represented instant. This matches the Protobuf Timestamp contract and Java’s floorDiv / floorMod semantics.
  • The complete production path was traced from CDC column packing in DataRecordResultConvertUtils.java:52, through socket delivery, to client reconstruction in ProtobufAnyValueConverter.java:84. Negative seconds with non-negative nanos are consumed correctly.
  • ColumnValueConvertUtilsTest.java:172 independently covers the java.sql.Timestamp and java.util.Date branches. Each parameterized invocation calls the public converter once and verifies both the nanos range and millisecond round trip.
  • Both previous blockers are fixed by 990bccb3: the cases are parameterized with name = "{0}", and the prohibited line comments were removed.
  • Non-negative inputs remain unchanged. The replacement adds no I/O, allocation, synchronization, public-contract, dependency, or complexity regression.

Coverage

  • Candidate: Public PR #39611 at head 990bccb36dd5b8e7f13a7e403e7d95dd586848d4, based on 2e1eb7d8a1f1db529c2079138dcc8658636d8fcf. The final refresh confirmed the head, discussion, and file list were unchanged.
  • Requirements: PR description and current public review discussion; no linked issue was present.
  • Files accounted for: Both authoritative GitHub-listed files. Their downloaded blob hashes matched GitHub, and a complete base/head source comparison found exactly these two changed files.
  • Behavior clusters: java.sql.Timestamp second flooring while preserving nanos; plain Date second flooring and non-negative nanos derivation.
  • Review lenses: Root cause and behavior, blast radius and contracts, tests/runtime/operations, protocol compatibility, performance, documentation, dependencies, and style.
  • Verification: Static source, contract, scope, and full-path verification completed. Local Maven execution was not performed because the environment did not authorize executing build plugins and tests from the public fork; no outcome-sensitive runtime fact remained unresolved.
  • Unresolved gaps: None that could change the blocker set. The final convergence pass found no new independent candidates.
  • CI scope: Code-correctness review only; GitHub CI was not queried or reviewed.

@terrymanu
terrymanu merged commit a269566 into apache:master Aug 30, 2026
28 of 29 checks passed
@terrymanu terrymanu added this to the 5.5.4 milestone Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@terrymanu terrymanu terrymanu approved these changes

Assignees

No one assigned

Labels

feature: pipeline type: enhancement

Projects

None yet

Milestone

5.5.4

Development

Successfully merging this pull request may close these issues.

2 participants

@PDGGK @terrymanu