fix: model interval qualifier as a structured property - #2456
Conversation
|
Thank you for your work and effort, but I have concerns on this one:
I really do appreciate your work and I do not want to revert it later when more comprehensive support was needed. (I myself have very little interest in DDLs, but everything about DMLs and Queries will catch my deeper interest.) |
Represent the SQL interval qualifier (field [TO field] [(precision)], e.g. DAY, HOUR TO MINUTE, DAY(9) TO SECOND, SECOND(2, 4)) as a structured IntervalExpression.IntervalQualifier, and consume it consistently in the three places it appears: - DML interval literals: SELECT INTERVAL '1' HOUR TO MINUTE (IntervalExpression / IntervalExpressionWithoutInterval) - DDL column types: CREATE TABLE t (c interval hour to minute) (ColDataType) - cast target types: CAST(x AS INTERVAL DAY TO SECOND) (reuses ColDataType) Previously the qualifier was stored as a single String and only the bare one-token form worked; field-TO-field, leading-field precision and fractional-seconds precision all failed to parse in every context. The qualifier is now a first-class property of both IntervalExpression and ColDataType rather than text appended to the data type string, so it round-trips losslessly and can be inspected structurally. This makes the long-standing Oracle interval01.sql (postfix day(9) to second) and interval03.sql (full qualifier matrix including second(2,4)) parse and de-parse, moving them to expected successes. Fixes JSQLParser#1728 Signed-off-by: 付典 <fudianchn@gmail.com>
ccb82d1 to
f272f1b
Compare
Signed-off-by: 付典 <fudianchn@gmail.com>
Signed-off-by: 付典 <fudianchn@gmail.com>
|
You are right on all three points. I rewrote the PR accordingly.
To make sure this is not reverted later: previously only the bare one-token form ( I left the old |
|
Your work and positivity is deeply appreciated, thank you! |
|
One recommendation: Please keep an eye also on |
|
Thanks for the heads-up on Round 1: low-spec and dedicated mid-spec runs used different sample counts ( The low-spec run was within the reported JMH uncertainty. Since that machine was not quiet, I also ran the comparison on a dedicated 32c/64g server.
Round 2: to keep the sample count consistent across both machines, I re-ran everything at
Across both rounds and both machines the difference stays within the reported JMH uncertainty, and on the quiet machine (reported uncertainty below 1%) master and this branch are effectively identical. The new grammar branches are guarded by semantic-predicate For completeness: the Done. |
|
That is perfectly fine and what we are aiming for. Only when it suddenly becomes 20% slower w/o a good explanation, we would worry. |
|
Makes sense, I'll treat the benchmark as part of the routine for any interval-related changes going forward, and I'm happy to follow up on this feature (or adjacent query/DML parsing) whenever there's more to cover or refine. |
Signed-off-by: 付典 <fudianchn@gmail.com>
|
This follow-up focuses on compatibility in the public I added additional regression cases for the public
These tests exposed two compatibility issues in the previous implementation. The JMH benchmark was run with identical parameters (
The reported JMH uncertainty ranges overlap in both runs, with no measurable performance regression. |
What
Rewrite of this PR based on @manticore-projects's feedback. The interval qualifier (
field [TO field] [(precision)], e.g.DAY,HOUR TO MINUTE,DAY(9) TO SECOND,SECOND(2, 4)) is now modeled as a structuredIntervalExpression.IntervalQualifierand consumed consistently everywhere it appears, instead of being appended to the data type string.The previous version appended the qualifier text to
ColDataType.dataType. That is reverted: the qualifier is now a first-class property of bothIntervalExpressionandColDataType.Addressing the review points
IntervalExpression.IntervalQualifier(leading field, optional leading precision, optional trailing field, optional fractional-seconds precision) is the structured qualifier, attached toIntervalExpressionviagetIntervalQualifier()and toColDataTypeviagetIntervalQualifier(). It is no longer bare text.field TO field, leading-field precision (DAY(9)), and fractional-seconds precision (SECOND(2, 4)). The sameIntervalQualifier()grammar rule is shared by all three consumers (DML literals, DDL column types, cast targets).CAST AS INTERVAL. This also turns the two long-standing Oracle failuresinterval01.sql((expr) day(9) to second) andinterval03.sql(full matrix includingsecond(2,4)) green, moving them to expected successes.Contexts fixed (all three, same rule)
SELECT INTERVAL '1' HOUR TO MINUTE(IntervalExpression)SELECT (systimestamp - order_date) DAY(9) TO SECOND FROM orders(IntervalExpressionWithoutInterval)CREATE TABLE t (c interval hour to minute)(ColDataType)SELECT CAST(x AS INTERVAL DAY TO SECOND)(reusesColDataType)Previously only the bare one-token form (
INTERVAL 1 DAY) parsed; field-TO-field, leading-field precision and fractional-seconds precision failed to parse in every context.How
IntervalExpression.IntervalQualifier(4 nullable fields, valueequals/hashCode,toStringrenderingDAY/DAY TO SECOND/DAY(9) TO SECOND/SECOND(2, 4)).IntervalQualifier()consuming<K_DATE_LITERAL> [(<p>[, <fp>])] [TO <K_DATE_LITERAL> [(<fp>)]].IntervalExpressionkeeps itsgetIntervalType()/setIntervalType(String)for backwards compatibility (still used by the non-standard single-identifier field like MySQLINTERVAL 1 foo), and addsgetIntervalQualifier()/setIntervalQualifier().ColDataTypegains anintervalQualifierfield, rendered intoString()and included inequals/hashCode.ExpressionDeParserrenders the structured qualifier when present.ColDataType()consumes the qualifier only when the matched type isINTERVALand is immediately followed by aK_DATE_LITERAL, gated by a semantic-predicateLOOKAHEAD, so other types and the existinginterval (2)precision form are unaffected.Trade-offs
IntervalQualifieracceptsK_DATE_LITERAL(the six standard fields) as field words. The DML prefix path additionally keeps the single-S_IDENTIFIERfallback for non-standard usage likeINTERVAL 1 foo; the postfix and DDL paths do not, to avoid grabbing arbitrary trailing identifiers (this is what originally preventedprediction(... cost model using ...)/xmltable(... passing warehouses.col ...)from mis-parsing).SECOND). Consistent with JSQLParser being a syntax parser, not a semantic checker.Testing
IntervalExpressionTestcases (DML): single field, field TO field, leading-field precision, field TO field with precision,SECOND(2, 4), structural AST assertion that the qualifier is attached to the interval, Oracle postfix form.ColDataTypeTestcases (DDL + CAST):interval hour to minutecolumn type, structural AST assertion on the column, bareinterval(2)still works,CAST AS INTERVAL DAY TO SECOND.interval01.sqlandinterval03.sqlmoved to expected successes (removed their stale@FAILUREannotations)../gradlew spotlessApply+./gradlew testgreen locally (4710 testcases, 0 failures).Fixes #1728