Skip to content

fix: model interval qualifier as a structured property - #2456

Open
fudianchn wants to merge 4 commits into
JSQLParser:masterfrom
fudianchn:fix/interval-type-qualifier-1728
Open

fix: model interval qualifier as a structured property#2456
fudianchn wants to merge 4 commits into
JSQLParser:masterfrom
fudianchn:fix/interval-type-qualifier-1728

Conversation

@fudianchn

@fudianchn fudianchn commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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 structured IntervalExpression.IntervalQualifier and 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 both IntervalExpression and ColDataType.

Addressing the review points

  1. Qualifier is a property of the Interval — done. IntervalExpression.IntervalQualifier (leading field, optional leading precision, optional trailing field, optional fractional-seconds precision) is the structured qualifier, attached to IntervalExpression via getIntervalQualifier() and to ColDataType via getIntervalQualifier(). It is no longer bare text.
  2. Do it right/complete — the qualifier now covers the full SQL-standard form: single field, field TO field, leading-field precision (DAY(9)), and fractional-seconds precision (SECOND(2, 4)). The same IntervalQualifier() grammar rule is shared by all three consumers (DML literals, DDL column types, cast targets).
  3. Tests were too lean / DML — interval qualifiers are now tested across all contexts: DML literals, the Oracle postfix form, DDL column types, and CAST AS INTERVAL. This also turns the two long-standing Oracle failures interval01.sql ((expr) day(9) to second) and interval03.sql (full matrix including second(2,4)) green, moving them to expected successes.

Contexts fixed (all three, same rule)

  • DML literal: SELECT INTERVAL '1' HOUR TO MINUTE (IntervalExpression)
  • Oracle postfix: SELECT (systimestamp - order_date) DAY(9) TO SECOND FROM orders (IntervalExpressionWithoutInterval)
  • DDL column type: CREATE TABLE t (c interval hour to minute) (ColDataType)
  • cast target: SELECT CAST(x AS INTERVAL DAY TO SECOND) (reuses ColDataType)

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

  • New nested type IntervalExpression.IntervalQualifier (4 nullable fields, value equals/hashCode, toString rendering DAY / DAY TO SECOND / DAY(9) TO SECOND / SECOND(2, 4)).
  • New shared grammar production IntervalQualifier() consuming <K_DATE_LITERAL> [(<p>[, <fp>])] [TO <K_DATE_LITERAL> [(<fp>)]].
  • IntervalExpression keeps its getIntervalType()/setIntervalType(String) for backwards compatibility (still used by the non-standard single-identifier field like MySQL INTERVAL 1 foo), and adds getIntervalQualifier()/setIntervalQualifier().
  • ColDataType gains an intervalQualifier field, rendered in toString() and included in equals/hashCode.
  • ExpressionDeParser renders the structured qualifier when present.
  • ColDataType() consumes the qualifier only when the matched type is INTERVAL and is immediately followed by a K_DATE_LITERAL, gated by a semantic-predicate LOOKAHEAD, so other types and the existing interval (2) precision form are unaffected.

Trade-offs

  • IntervalQualifier accepts K_DATE_LITERAL (the six standard fields) as field words. The DML prefix path additionally keeps the single-S_IDENTIFIER fallback for non-standard usage like INTERVAL 1 foo; the postfix and DDL paths do not, to avoid grabbing arbitrary trailing identifiers (this is what originally prevented prediction(... cost model using ...) / xmltable(... passing warehouses.col ...) from mis-parsing).
  • No semantic validation of field combinations or precision ranges (e.g. PG allows precision only for SECOND). Consistent with JSQLParser being a syntax parser, not a semantic checker.

Testing

  • New IntervalExpressionTest cases (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.
  • New ColDataTypeTest cases (DDL + CAST): interval hour to minute column type, structural AST assertion on the column, bare interval(2) still works, CAST AS INTERVAL DAY TO SECOND.
  • interval01.sql and interval03.sql moved to expected successes (removed their stale @FAILURE annotations).
  • ./gradlew spotlessApply + ./gradlew test green locally (4710 testcases, 0 failures).

Fixes #1728

@manticore-projects

manticore-projects commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Thank you for your work and effort, but I have concerns on this one:

  1. my understanding is, that Interval Qualifier should be a property of an Interval, am I wrong on this?
  2. if we do this, we should do it right/complete
  3. the test cases are a bit lean, only showing CREATE ... and column types, while this would be used in all kind selects

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>
@fudianchn
fudianchn force-pushed the fix/interval-type-qualifier-1728 branch from ccb82d1 to f272f1b Compare August 10, 2026 03:29
@fudianchn fudianchn changed the title fix: parse INTERVAL column type with time-unit qualifier fix: model interval qualifier as a structured property Aug 10, 2026
Signed-off-by: 付典 <fudianchn@gmail.com>
Signed-off-by: 付典 <fudianchn@gmail.com>
@fudianchn

Copy link
Copy Markdown
Contributor Author

You are right on all three points. I rewrote the PR accordingly.

  1. The qualifier is now a property of the interval. New IntervalExpression.IntervalQualifier (leading field, optional leading precision, optional trailing field, optional fractional-seconds precision) is attached via getIntervalQualifier() on IntervalExpression and on ColDataType. The previous version appended the qualifier text to ColDataType.dataType; that is reverted.

  2. It now covers the full SQL-standard form: single field, field TO field, leading-field precision (DAY(9)), fractional-seconds precision (SECOND(2, 4)). A single shared IntervalQualifier() grammar rule is consumed in all three places: DML literals (SELECT INTERVAL '1' HOUR TO MINUTE), the Oracle postfix form ((expr) DAY(9) TO SECOND), and column/cast types.

  3. Tests now cover all contexts: DML literals, postfix, DDL column types, and CAST AS INTERVAL, with AST assertions that the qualifier is attached as a structured property. This also turns the two long-standing Oracle failures green: interval01.sql ((expr) day(9) to second) and interval03.sql (the full qualifier matrix including second(2,4)), which failed since 2021.

To make sure this is not reverted later: 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. The rewrite parses and round-trips all of them. Local ./gradlew test is green (4710 testcases, 0 failures); CI (Gradle Check on 3 platforms, Maven Verify, Codacy) is green too.

I left the old getIntervalType()/setIntervalType(String) in place for the non-standard single-identifier field like MySQL INTERVAL 1 foo, so existing callers keep working. Happy to drop it or adjust anything.

@manticore-projects

Copy link
Copy Markdown
Contributor

Your work and positivity is deeply appreciated, thank you!
Its an impressive PR so please give me some time to study it well. Thanks and cheers, mate!

@manticore-projects

Copy link
Copy Markdown
Contributor

One recommendation: Please keep an eye also on gradle jmh in order to avoid new hot paths or performance degradation (we had such in the past, when new features completely destroyed the performance).

@fudianchn

fudianchn commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the heads-up on gradle jmh. I ran the parseSQLStatements benchmark (on performance.sql) on two servers to check for regressions, comparing clean master against this branch with identical parameters (JDK 17.0.20). I did two rounds of measurements and report both. JMH uncertainty is listed as master / this branch.

Round 1 — low-spec server (16 cores / 32 GB, mixed load), -f 3 -wi 3 -i 6 (18 samples):

Server master This branch JMH uncertainty Samples
16c/32g, mixed load 8.903 ms/op 9.086 ms/op ±0.249 / ±0.270 ms/op 18

The ~2% gap was within the reported JMH uncertainty, but since that machine was not quiet I re-ran on a dedicated mid-spec server (32 cores / 64 GB, idle), -f 5 -wi 5 -i 10 (50 samples):

Server master This branch JMH uncertainty Samples
32c/64g, idle 3.645 ms/op 3.631 ms/op ±0.030 / ±0.029 ms/op 50

Round 2 — to keep the sample count consistent across both machines, I re-ran everything at -f 10 -wi 5 -i 10 (100 samples each).

Low-spec server (16 cores / 32 GB, mixed load):

Server master This branch JMH uncertainty Samples
16c/32g, mixed load 9.648 ms/op 9.331 ms/op ±0.344 / ±0.273 ms/op 100

Mid-spec server (32 cores / 64 GB, idle):

Server master This branch JMH uncertainty Samples
32c/64g, idle 3.635 ms/op 3.638 ms/op ±0.022 / ±0.022 ms/op 100

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 LOOKAHEADs (getToken(1).kind == K_DATE_LITERAL after an INTERVAL type, etc.), so they only fire on the interval path and add at most one token-kind check elsewhere.

For completeness: the jmh benchmark itself is not part of the default check chain (only spotbugsJmh/checkstyleJmh static analysis of the benchmark sources is), so I ran it explicitly as above. I'll keep running it on future interval-related changes.

Done.

@manticore-projects

Copy link
Copy Markdown
Contributor

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.

@fudianchn

Copy link
Copy Markdown
Contributor Author

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>
@fudianchn

fudianchn commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

This follow-up completes the compatibility work for #1728, which reported that CREATE TABLE ... len interval hour to minute could not be parsed. The structured qualifier introduced in this PR fixes that syntax while preserving the existing public IntervalExpression API.

I added additional regression cases for the public IntervalExpression API. They cover:

  • getIntervalType() for single-field and field-to-field qualifiers;
  • setIntervalType(...) replacing a structured qualifier;
  • setIntervalQualifier(...) replacing the legacy interval type;
  • round-trip rendering through both toString() and the expression deparser.

These tests exposed two compatibility issues in the previous implementation. getIntervalType() returned null for parsed structured qualifiers, and the two setters could leave the legacy getter and rendered SQL out of sync. The implementation now keeps the structured qualifier as the canonical parsed representation, exposes its SQL form through the legacy getter, and makes the legacy and structured representations mutually exclusive when either setter is used.

The JMH benchmark was run with identical parameters (-f 10 -wi 5 -i 10) on performance.sql using JDK 17.0.20. JMH uncertainty is listed as master / this branch:

Server master This branch JMH uncertainty Samples
16c/32g, mixed load 9.648 ms/op 9.331 ms/op ±0.344 / ±0.273 ms/op 100
32c/64g, via ssh lan -> ssh dev 3.680 ms/op 3.671 ms/op ±0.025 / ±0.026 ms/op 100

The reported JMH uncertainty ranges overlap in both runs, with no measurable performance regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] JSQLParser 4.5 : Postgres : fails to parse interval hour to minute

2 participants