Skip to content

fix: correct WEEK interval literal duration in calciteLiteralToDruidLiteral (#18665) - #19906

Open
waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/week-interval-duration
Open

fix: correct WEEK interval literal duration in calciteLiteralToDruidLiteral (#18665)#19906
waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/week-interval-duration

Conversation

@waterWang

Copy link
Copy Markdown

Description

Fixes #18665

INTERVAL 1 WEEK in SQL resolves to PT1H (1 hour) instead of P7D (7 days).

SELECT MILLIS_TO_TIMESTAMP(0) + INTERVAL 1 WEEK

Returns 1970-01-01T01:00:00.000Z instead of 1970-01-08T00:00:00.000Z.

Root Cause

Calcite has a known quirk where WEEK interval literals are stored as 1 hour in the INTERVAL_DAY_TIME family. The INTERVAL 1 WEEK literal's RexLiteral.value() returns 3600000 (1 hour in ms) instead of 604800000 (7 days in ms).

Fix

In calciteLiteralToDruidLiteral, detect the WEEK qualifier on the SqlIntervalQualifier and multiply the stored millisecond value by 7 × 24 to convert from the incorrect 1-hour representation to the correct 7-day duration.

Impact

All INTERVAL N WEEK expressions now correctly resolve to N weeks instead of N hours.
The fix applies to all contexts where INTERVAL_DAY_TIME literals are converted to Druid expressions: timestamp arithmetic (+/-), TIMESTAMPDIFF, and other interval-using operations.

Verification

SELECT MILLIS_TO_TIMESTAMP(0) + INTERVAL 1 WEEK
-- Before: 1970-01-01T01:00:00.000Z
-- After:  1970-01-08T00:00:00.000Z

…iteral (apache#18665)

Calcite has a known quirk where WEEK interval literals are stored as 1 hour
in the INTERVAL_DAY_TIME family. This causes `INTERVAL 1 WEEK` to resolve
to PT1H (1 hour) instead of P7D (7 days).

Detect the WEEK qualifier in the SqlIntervalQualifier and convert the
millisecond value to 7 days (multiply by 7 * 24).
@FrankChen021

Copy link
Copy Markdown
Member

this problem is supposed to be fixed by #19370 , but looks like it was not fix
can you add some test cases in this PR?

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity Findings
P0 0
P1 1
P2 0
P3 0
Total 1

Reviewed 1 of 1 changed files. Found one P1 correctness issue causing WEEK intervals to be over-scaled.


This is an automated review by Codex GPT-5.6-Luna(max)

// Detect the WEEK qualifier and convert the value to 7 days.
final SqlIntervalQualifier intervalQualifier = rexNode.getType().getIntervalQualifier();
if (intervalQualifier != null && intervalQualifier.getStartUnit() == TimeUnit.WEEK) {
milliseconds = milliseconds * 7 * 24;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Avoid rescaling already-correct WEEK literals

This branch uses Calcite 1.41, which already converts WEEK intervals to millisecond values. A narrow probe produced 604800000 for INTERVAL '1' WEEK; multiplying by 7 * 24 changes it to 101606400000 ms, making existing quoted and unquoted WEEK expressions 168 times too large. Apply the workaround only to the pre-1.38 representation or remove it.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

INTERVAL 1 WEEK in SQL is wrong interval

2 participants