fix: model interval qualifier as a structured property

[fudianchn]

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]

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.)

[fudianchn]

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]

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]

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]

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 and dedicated mid-spec runs used different sample counts (-f 3 -wi 3 -i 6 for 16c/32g, 18 samples; -f 5 -wi 5 -i 10 for 32c/64g, 50 samples).

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.

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
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).

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, 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]

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]

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.

[fudianchn]

This follow-up focuses on compatibility in the public IntervalExpression API. The structured qualifier introduced in this PR remains the canonical parsed representation while existing callers receive consistent getter and rendering behavior.

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	8.681 ms/op	8.804 ms/op	±0.180 / ±0.280 ms/op	100
32c/64g, idle	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.

[manticore-projects]

Excellent work! Three things before merging, mainly public API which we can't repair later:

1. IntervalQualifier sits in the wrong place. Nested in IntervalExpression, but ColDataType (DDL) has to import it from the expression package. Please promote it to a top level net.sf.jsqlparser.expression.IntervalQualifier.

2. Two representations of the same thing. getIntervalType() now returns intervalQualifier.toString() and the setters null each other out, so setIntervalType(getIntervalType()) silently downgrades a structured qualifier into a string, and the new ExpressionDeParser branch is redundant. If the qualifier is canonical, and it should be, please mark the legacy getter/setter @Deprecated and keep them only for the non standard INTERVAL 1 foo case. Right now both look equally supported and people will pick the wrong one.

3. Two remarks on the IntervalQualifier() production. Both optionals use the default LOOKAHEAD(1), so a ( after a field word which is not a precision becomes a hard ParseException instead of backtracking, LOOKAHEAD(2) costs us nothing there. And fractionalPrecision is written by two alternatives, so SECOND(2, 4) TO SECOND(3) silently drops the 4. Invalid SQL anyway, but I prefer rejecting input over quietly losing parts of it.

Thanks again, and for running the benchmarks without me having to ask twice. Cheers, mate!

[fudianchn]

All three points are addressed. I also squashed the PR into a single commit (a604618).

1. IntervalQualifier promoted to a top-level type. It now lives at net.sf.jsqlparser.expression.IntervalQualifier. ColDataType and the grammar reference it directly; the cross-package import from the expression package is gone.

2. The qualifier is canonical; the legacy API is @Deprecated. getIntervalType(), setIntervalType() and withIntervalType() are deprecated and kept only for the non-standard single-identifier form (e.g. MySQL INTERVAL 1 foo). getIntervalType() returns the raw legacy field only, so it returns null whenever a structured qualifier is present (for INTERVAL '1' DAY it now returns null instead of synthesizing "DAY"). Legacy getter returns the raw field -> setIntervalType(getIntervalType()) can no longer downgrade a structured qualifier into a string -> the ExpressionDeParser qualifier branch and legacy branch now serve distinct cases (structured qualifier vs. non-standard string) and are no longer redundant.

3. IntervalQualifier() production. Both optionals use LOOKAHEAD(2). The leading two-argument precision (SECOND(2, 4)) and the trailing-field precision (TO SECOND(3)) are now distinct tokens; SECOND(2, 4) TO SECOND(3) is rejected with a ParseException instead of silently dropping the 4. The rejection is placed as the terminal statement so the production does not complete normally and no unreachable guard is emitted.

Verification.

• Local: ./gradlew spotlessApply test checkstyleMain checkstyleTest spotbugsMain green; the full test suite passes. Added regressions: non-standard INTERVAL 1 foo keeps the legacy getter and attaches no qualifier; SECOND(2, 4) TO SECOND(3) is rejected.
• CI: Gradle Check (macOS/Linux/Windows), Maven Verify, Codacy all green.

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

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

performance.sql contains no INTERVAL statements, so the changed grammar path is not exercised here; the delta stays within the reported uncertainty and reflects no code-path change. The shared ColDataType interval gate (the semantic-predicate LOOKAHEAD) is unchanged, so non-INTERVAL types are unaffected.

[fudianchn]

@manticore-projects One scope point to confirm before merge, since it is a public-API behavior change. Every K_DATE_LITERAL field now routes through IntervalQualifier, including the bare single-field form: INTERVAL '1' DAY -> qualifier set, getIntervalType() returns null, value on getIntervalQualifier().getLeadingField(). The legacy string API is reserved exclusively for the non-standard S_IDENTIFIER form (INTERVAL 1 foo). This is point 2 implemented literally; flagging it because downstream callers of getIntervalType() on previously-parseable standard intervals will now see null. Should that be called out anywhere beyond the @Deprecated javadoc (release notes / changelog)?

[manticore-projects]

Thank you so so much!

Should that be called out anywhere beyond the @deprecated javadoc (release notes / changelog)?
I will update the README and WebSite accordingly, since you also closed a major "unsupported feature".