leaves12138 commented on code in PR #919:
URL: https://github.com/apache/paimon-rust/pull/919#discussion_r4077987314
##########
crates/paimon/src/spec/core_options.rs:
##########
@@ -1614,6 +1623,65 @@ 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, LocalResult, 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 UTC instant during an overlap.
+ // Chrono's LocalResult candidates are not necessarily ordered by instant
+ // (notably for chrono::Local on some platforms), so compare explicitly.
+ match zone.from_local_datetime(&datetime) {
+ LocalResult::Single(timestamp) => return
Ok(timestamp.timestamp_millis()),
+ LocalResult::Ambiguous(first, second) => {
+ return Ok(first.timestamp_millis().min(second.timestamp_millis()));
Review Comment:
[P2] Reject invalid overlap candidates before taking the minimum
The original `01:30` case is fixed, but the end of the overlap still
resolves incorrectly through `chrono::Local`. With `TZ=America/New_York`,
`scan.timestamp=2024-11-03 02:00:00` must resolve to **07:00 UTC
(1730617200000)**; this version resolves it to **06:00 UTC (1730613600000)**
instead. The same mismatch occurs at `02:00:00.999`, while `02:00:01` is
correct.
The locked Chrono local-zone implementation treats the overlap's upper
boundary as inclusive and returns an invalid DST candidate there. Taking the
minimum selects that invalid candidate: its 06:00 UTC instant converts back to
01:00 local time, not the requested 02:00.
I reproduced this through the SQL reader built from this commit: snapshot 1
at 05:00 UTC contains one row; snapshot 2 at 06:30 UTC adds a second row. The
string selector `2024-11-03 02:00:00` reads **1 row / snapshot 1**, whereas the
Java-equivalent `scan.timestamp-millis=1730617200000` correctly reads **2 rows
/ snapshot 2**. Similar parser mismatches occur at overlap endpoints in
Europe/Berlin, Europe/London, and Australia/Lord_Howe.
Please add the endpoint case to the real-Local/public-reader regression. If
retaining this implementation, validate ambiguous candidates by converting
their UTC instants back through the timezone and discard candidates that do not
reproduce the requested local datetime before choosing the earlier one.
Alternatively, delegate timezone disambiguation to a library such as Jiff's
`Compatible` strategy instead of maintaining these transition workarounds. I
independently checked Jiff against the Java conversion rules for nine
fold/gap/endpoint cases (including Lord Howe's half-hour change and Apia's
skipped day); all matched. The Paimon-specific input formats and millisecond
truncation should still be preserved.
--
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]
