gengliangwang commented on code in PR #56847:
URL: https://github.com/apache/spark/pull/56847#discussion_r3546928797
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7139,8 +7139,10 @@ object SQLConf {
val TIME_TRAVEL_AT_SYNTAX_ENABLED =
buildConf("spark.sql.timeTravel.atSyntax.enabled")
.doc("When true, a table name in a query or in table-reading APIs can
carry a time " +
- "travel suffix: 'name@v123' reads version 123 of the table. When
false, '@' in " +
- "table names fails at parse time.")
+ "travel suffix: 'name@v123' reads version 123 of the table, and " +
+ "'name@20240101000000000' (format yyyyMMddHHmmssSSS, interpreted in
the session " +
Review Comment:
The doc gives the format but not its granularity — a reader has to infer
that `SSS` means the resolution is millisecond, and that sub-millisecond time
travel isn't expressible here and needs `TIMESTAMP AS OF`. Worth one clause,
e.g. "…reads the table as of that timestamp, to millisecond precision (use
`TIMESTAMP AS OF` for finer granularity)."
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala:
##########
@@ -2223,6 +2234,28 @@ class PlanParserSuite extends AnalysisTest {
condition = "MULTIPLE_TIME_TRAVEL_SPEC",
parameters = Map.empty,
context = ExpectedContext(fragment = "TIMESTAMP AS OF '2019-01-29'",
start = 19, stop = 46))
+ checkError(
+ exception = parseException("SELECT * FROM t@20190129003758000 VERSION AS
OF 2"),
+ condition = "MULTIPLE_TIME_TRAVEL_SPEC",
+ parameters = Map.empty,
+ context = ExpectedContext(fragment = "VERSION AS OF 2", start = 34, stop
= 48))
+ checkError(
+ exception = parseException("SELECT * FROM t@20190129003758000 TIMESTAMP
AS OF '2019-01-29'"),
+ condition = "MULTIPLE_TIME_TRAVEL_SPEC",
+ parameters = Map.empty,
+ context = ExpectedContext(fragment = "TIMESTAMP AS OF '2019-01-29'",
start = 34, stop = 61))
+
+ checkError(
+ exception = parseException("SELECT * FROM t@123"),
Review Comment:
The 17-digit contract is thinly tested at both boundaries here, so a
regression in it would likely go unnoticed:
- **Negative:** the length check is only ever hit by `t@123` (3 digits),
which no user would type. The realistic wrong-length mistakes —
second-precision `t@20190129003758` (14 digits) and date-only `t@20190129` (8
digits) — aren't covered. They're correctly rejected today (they fail
`text.length != format.length`), but nothing pins that the shorthands a user
will actually reach for fail with a clean
`INVALID_TIME_TRAVEL_TIMESTAMP_FORMAT`.
- **Positive:** every `@<ts>` literal in the suites ends in `...000`, and
the expected micros come from `"2019-01-29 00:37:58"` (no fractional part), so
the `SSS` arithmetic (`substring(14,17) * NANOS_PER_MILLIS` in
`AstBuilder.parseAtSyntaxTimestamp`) is never actually observed — a wrong
multiplier or off-by-one substring bound would still pass because `000 *
anything == 0`.
Adding the two negative cases above plus one non-zero-millis positive (e.g.
`@20190129003758123` → `Literal` of `"2019-01-29 00:37:58.123"`) would lock
down both ends.
--
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]
