PDGGK opened a new pull request, #39611:
URL: https://github.com/apache/shardingsphere/pull/39611
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:
```java
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:
- [x] My code follows the [code of
conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/)
of this project.
- [x] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the
pull request.
- [x] 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.
- [x] 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](https://shardingsphere.apache.org/community/en/involved/contribute/contributor/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
