leaves12138 commented on code in PR #919:
URL: https://github.com/apache/paimon-rust/pull/919#discussion_r4077861243
##########
crates/paimon/src/spec/core_options.rs:
##########
@@ -1614,6 +1623,59 @@ impl<'a> CoreOptions<'a> {
}
}
+/// Java DateTimeUtils accepts a date, a space-separated timestamp, or an ISO
+/// local timestamp (whose seconds are optional). It truncates to milliseconds
+/// and resolves the date/time in the process's default time zone.
+fn parse_scan_timestamp(value: &str, zone: &impl chrono::TimeZone) ->
crate::Result<i64> {
+ use chrono::{Days, NaiveDate, NaiveDateTime, Offset, TimeZone, Timelike};
+
+ let datetime = NaiveDateTime::parse_from_str(value, "%Y-%m-%d %H:%M:%S%.f")
+ .or_else(|_| NaiveDateTime::parse_from_str(value,
"%Y-%m-%dT%H:%M:%S%.f"))
+ .or_else(|_| NaiveDateTime::parse_from_str(value, "%Y-%m-%dT%H:%M"))
+ .or_else(|_| {
+ NaiveDate::parse_from_str(value, "%Y-%m-%d")
+ .map(|date| date.and_hms_opt(0, 0, 0).unwrap())
+ })
+ .map_err(|error| crate::Error::DataInvalid {
+ message: format!("Invalid value for {SCAN_TIMESTAMP_OPTION}:
'{value}'"),
+ source: Some(Box::new(error)),
+ })?;
+ // Chrono also accepts leap seconds and fractions longer than nanoseconds;
+ // Java's local timestamp parser does not.
+ if datetime.nanosecond() >= 1_000_000_000
+ || value
+ .rsplit_once('.')
+ .is_some_and(|(_, fraction)| fraction.len() > 9)
+ {
+ return Err(crate::Error::DataInvalid {
+ message: format!("Invalid value for {SCAN_TIMESTAMP_OPTION}:
'{value}'"),
+ source: None,
+ });
+ }
+ // LocalDateTime.atZone chooses the earlier instant during an overlap.
+ if let Some(timestamp) = zone.from_local_datetime(&datetime).earliest() {
Review Comment:
[P2] Resolve local-time overlaps by the actual UTC instant
The production `chrono::Local` path does not match the
`arrow_array::timezone::Tz` used in the new transition test. With the locked
Chrono 0.4.45 on this Linux host, `TZ=America/New_York` and
`scan.timestamp=2024-11-03 01:30:00`, this `.earliest()` call resolves to
**06:30 UTC (1730615400000)**, whereas Java resolves to **05:30 UTC
(1730611800000)**. The local-zone overlap candidates are not ordered as this
call assumes.
I reproduced the observable error through the actual `SQLContext` reader
built from this commit:
- Snapshot 1 at `1730610000000` (05:00 UTC), containing row 1.
- Snapshot 2 at `1730613600000` (06:00 UTC), adding row 2.
- `SET 'paimon.scan.timestamp' = '2024-11-03 01:30:00'` followed by `SELECT`
returns **2 rows / snapshot 2**.
- Resetting that option and setting
`paimon.scan.timestamp-millis=1730611800000` returns the Java-expected **1 row
/ snapshot 1**.
This silently includes data committed after the Java-equivalent time-travel
cutoff. Please resolve ambiguous local times using the actual instants rather
than relying on the candidate order, and add a regression through
`chrono::Local` (ideally the public reader in a subprocess with
`TZ=America/New_York`). The current transition test passes because it exercises
a different timezone implementation.
--
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]
