SEPURI-SAI-KRISHNA opened a new pull request, #13043:
URL: https://github.com/apache/seatunnel/pull/13043
Yes, glad to contribute to Apache SeaTunnel.
### Purpose of this pull request
Closes #13042.
The Lance sink declares `TIMESTAMP` columns with microsecond precision, and
the documentation promises it, but `BaseTypeWriter.convertToEpochMicro` went
through milliseconds and dropped the last three digits of every value.
**The fix.** `LocalDateTime` and `java.sql.Timestamp` values are now
converted from their `Instant` directly: whole seconds times 1,000,000 plus
`getNano() / 1000`. `getEpochSecond()` rounds down and `getNano()` is never
negative, so values before 1970 convert exactly too. `java.util.Date` only
carries milliseconds and is unchanged.
**A second change, so the fix does not regress another path.** The writer
adopts the schema of a dataset that already exists
(`LanceSinkWriter.initializeDataset`), so it can meet a millisecond timestamp
column it did not create. `TimestampTypeWriter` converted to milliseconds with
`epochMicro / 1000`, which rounds toward zero. That was harmless while every
value was a whole millisecond. Once the sub-millisecond digits are kept, a
value before 1970 would round up instead: `1969-12-31T23:59:59.999500` on a UTC
JVM would be written as `1970-01-01T00:00:00`. Both millisecond sites, the
plain column and the list element, now use `Math.floorDiv`, which writes what
`dev` writes today for those values.
Unchanged: the zone the value is encoded in, the declared schema, and every
option.
### Does this PR introduce any user facing change?
Yes. A `TIMESTAMP` written to a microsecond column now keeps its
sub-millisecond digits, as documented. Rows already in a dataset are not
rewritten. The entry is added to `incompatible-changes.md` in en and zh.
One edge: a value more than about 292,000 years from 1970 now fails with
`ArithmeticException`, where `* 1000` used to overflow silently.
### How was this patch tested?
**Unit tests**, two new cases in the existing `FragmentConverterTest`,
through `FragmentConverter.convertToVectorSchemaRoot` and the real writer
dispatch:
- `shouldKeepTheMicrosecondsOfATimestamp`: the schema
`SchemaUtils.convertSchema` builds when the sink creates the dataset, with a
recent value, a value before 1970 and a `java.sql.Timestamp`.
- `shouldRoundAMillisecondTimestampBeforeTheEpochDown`: a millisecond column
and a millisecond list, as an existing dataset could have.
Mutation checks, each part reverted on its own:
| reverted to `dev` | result |
| --- | --- |
| `BaseTypeWriter` | `shouldKeepTheMicrosecondsOfATimestamp` fails |
| both `floorDiv` sites |
`shouldRoundAMillisecondTimestampBeforeTheEpochDown` fails |
| the list `floorDiv` site only | the same test fails at the list assertion |
`FragmentConverterTest` passes with `-Duser.timezone` set to UTC,
Asia/Shanghai, America/New_York, Asia/Kolkata and America/St_Johns.
**E2E.** `LanceIT.testTimestampKeepsItsMicroseconds` runs
`fake_timestamp_to_lance.conf`, which writes three rows with sub-millisecond
digits, then reads the dataset back in the test JVM and compares each id's
fraction of a second. Unlike the existing row count check, it fails rather than
skips when the dataset cannot be read, so a pass means the stored values were
checked. In a recent CI run of `LanceIT` on JDK 17 the test JVM did open the
dataset, so the read-back runs in CI.
Docker is not available to me, so the job itself has not run locally. I ran
the read-back method verbatim against a dataset written by the real
`LanceSinkWriter` through native Lance: `{1=123456, 2=250, 3=999500}` with this
change and `{1=123000, 2=0, 3=999000}` on `dev`. `LanceIT.java` compiles.
**Build checks**, JDK 17:
- `./mvnw test -pl seatunnel-connectors-v2/connector-lance`, with the
`surefire.jvm.args` the unit-test CI lane passes: 39 tests, 0 failures.
- `./mvnw spotless:check` on `connector-lance` and `connector-lance-e2e`:
clean.
- `./mvnw -B -T 1 test -D"license.skipAddThirdParty"=true -pl
seatunnel-ci-tools`: 34 tests, 0 failures.
### Check list
* [X] Code changed are covered with tests, or it does not need tests for
reason
* [X] If any new Jar binary package adding in your PR, please add License
Notice according to
[License
Notice](https://github.com/apache/seatunnel/blob/dev/docs/en/contribution/new-license.md):
none added.
* [X] If necessary, please update the documentation to describe the new
feature: the sink documentation already promises microsecond precision;
`incompatible-changes.md` is updated in en and zh.
* [X] If you are contributing the connector code, please check that the
following files are updated:
1. Update
[plugin-mapping.properties](https://github.com/apache/seatunnel/blob/dev/plugin-mapping.properties):
not needed, no new connector.
2. Update the pom file of
[seatunnel-dist](https://github.com/apache/seatunnel/blob/dev/seatunnel-dist/pom.xml):
not needed, no new module.
3. Add ci label in
[label-scope-conf](https://github.com/apache/seatunnel/blob/dev/.github/workflows/labeler/label-scope-conf.yml):
not changed. `connector-lance` has no entry there today, and adding one is
outside this fix.
4. Add e2e test for your connector: `LanceIT` is extended.
5. Update [connector-v2
doc](https://github.com/apache/seatunnel/tree/dev/docs/en/connectors): not
needed, see above.
* [X] Update the `release-note`: not applicable, the file was removed from
`dev` in #9304.
--
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]
