ninglinLiu commented on PR #7215: URL: https://github.com/apache/datafusion-comet/pull/7215#issuecomment-6401967734
Thanks for the careful review and the measurements. I pushed 3605746 with the following changes. **Performance.** The `UTC`/`GMT`/`UT` prefix rule now runs before the region lookup again, and it is anchored instead of using `rfind_str`: the offset can only contain signs, digits and colons, so the rule trims that trailing run once and checks whether the text before it ends in `UTC`, `GMT` or `UT`. A name that follows a `/` is skipped, so `Etc/UTC` and `Etc/GMT+8` still go to the region lookup and `Etc/GMT+8` stays UTC-8. `rfind_str` had no other callers, so I removed it. I timed `timestamp_parser` in a release build (a temporary loop of 2M calls, best of three, on a shared 8-core VM, so the absolute numbers are higher than yours and have a few ns of noise; I built without LTO because the box runs out of memory with `codegen-units=1`): | Input | `main` | PR before | PR now | | --- | --- | --- | --- | | `2020-01-01 12:34:56 UTC` | 168 ns | 190 ns | 178 ns | | `2020-01-01 12:34:56Z` | 161 ns | 160 ns | 162 ns | | `2020-01-01 12:34:56` | 103 ns | 103 ns | 103 ns | | `2020-01-01 12:34:56 GMT+8` | 315 ns | 445 ns | 182 ns | | `2020-01-01T12:34:56+08:00` | 1051 ns | 741 ns | 352 ns | | `2020-01-01 12:34:56 PST` | 824 ns | 189 ns | 201 ns | | `2020-01-01 12:34:56 Europe/Moscow` | 1320 ns | 164 ns | 168 ns | `UTC` is back within about 10 ns of `main` and of `Z`, and `GMT+8` was also slower in the previous revision, which this fixes too. **One-digit hour in the seconds form.** `parse_sign_offset` now accepts a one- or two-digit hour with two-digit minutes and seconds, so `+8:00:00`, `-8:00:00`, `UTC+8:00:00` and `GMT-8:00:00` resolve. `+8:0:00` is still rejected. There are unit cases for each. **Offsets with non-zero seconds.** I filed #7224 and linked it from the comment in `tz_from_offset_secs`, from a unit case that pins `+08:00:30` to NULL, and from the compatibility guide. **Tests.** - New `timestamp_parser_short_id_summer_test` with July dates: `PST` is -07:00, `CST` -05:00 and `ECT` +02:00, while `EST`, `MST` and `HST` keep their fixed offsets. - New `java_short_ids_resolve_test` checks that every `JAVA_SHORT_IDS` target resolves through `Tz::from_str` and that the table still has 28 entries. - New SQL file `cast_string_to_timestamp_zone_ids.sql`, run with session time zones `UTC` and `America/Los_Angeles`. It is table-backed (Parquet), casts each value to both `TIMESTAMP` and `TIMESTAMP_NTZ`, runs `try_cast`, and repeats the valid set under `spark.sql.ansi.enabled=true` with `expect_error(CAST_INVALID_INPUT)` for `pst`, `+8:0:00` and `XYZ`. I put it in its own file so the ANSI setting doesn't leak into the rest of `cast.sql`; happy to move it if you prefer. `CometSqlFileTestSuite` passes for the new file on the default profile and on `-Pspark-4.1`, and for `cast.sql` on the default profile. With the previous revision's native code the new file fails on `+8:00:00`. `cargo test -p datafusion-comet-spark-expr --lib` (1299 tests), clippy with `-D warnings` and `cargo fmt` are clean. **Docs.** The "String to Timestamp" section now lists the supported suffix forms, the 28 short ids (split into the region-backed ones and the three fixed offsets), and region ids with and without a `/`. It also says zone ids are case sensitive, and that offsets with a non-zero seconds part are not supported, with a link to #7224. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
