Skip to content

fix(query): accept d/w suffixes in time_range, reject bad input - #292

Merged
EricAndrechek merged 7 commits into
mainfrom
bug/time_range-RFC3339
Jun 8, 2026
Merged

EricAndrechek merged 7 commits into
mainfrom
bug/time_range-RFC3339

Conversation

@taitelee

@taitelee taitelee commented Jun 6, 2026

Copy link
Copy Markdown
Member

Summary

time_range.since/until accept relative durations and sdk.md advertises day suffixes (7d), but Go's time.ParseDuration stops at hours — so "7d" failed to parse and fell through to ClickHouse as a raw DateTime literal, surfacing as an opaque Cannot read DateTime error (a retryable 500, per #271). This fixes it in the structured-query builder:

  • Day/week suffixes work. resolveTimeValue pre-expands Nd→N*24h and Nw→N*168h (7d→168h, 2w→336h, 1d12h→24h12h) before time.ParseDuration, so the documented ranges resolve correctly.
  • Invalid input fails closed. resolveTimeValue now returns an error for any value that is neither a relative duration nor an RFC3339 timestamp, and Build maps it to a 400 with a clear message instead of handing a raw string to ClickHouse.

Test plan

  • Added TestExpandDayWeek, TestResolveTimeValue_DayWeekSuffix, TestResolveTimeValue_Invalid, and a Build-level invalid time range since duration case; updated the three existing resolveTimeValue tests for the new (string, error) signature
  • Manual sanity: POST /v1/query with time_range.since: "7d" returns rows; "banana" returns 400

Related Issues

Closes #285
Relates to #271 (opaque ClickHouse error → 400)

@coderabbitai

coderabbitai Bot commented Jun 6, 2026 •

Copy link
Copy Markdown

Review Change Stack

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Free

Run ID: 8ea1d3dc-33c7-413f-bae6-91bdc4761077

📥 Commits

Reviewing files that changed from the base of the PR and between 099c654 and 365232d.

📒 Files selected for processing (1)
  • internal/query/ast.go

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Time range parameters now accept day/week relative durations (e.g., 7d, 2w) and return clear HTTP 400 errors for invalid/unparseable values.
  • Documentation

    • API and SDK docs updated to indicate RFC3339 timestamps and expanded relative-duration formats are accepted.
  • Tests

    • Added coverage for duration suffixes, day/week expansion, bucketing behavior, and proper error handling for invalid time inputs.

Walkthrough

This PR adds day/week duration suffixes to time range parameters, making since/until accept formats like 7d and 2w alongside RFC3339 timestamps. Parsing now fails fast with descriptive errors (resulting in HTTP 400) instead of deferring invalid values to ClickHouse.

Changes

Time Range Parsing Enhancement

Layer / File(s) Summary
Core time parsing implementation
internal/query/builder.go
Introduces expandDayWeek to convert d/w suffixes to hours, and restructures resolveTimeValue to return (string, error), rejecting any value that is neither a valid expanded duration nor a valid RFC3339Nano timestamp.
Time parsing tests
internal/query/builder_test.go
New tests for expandDayWeek suffix conversion, resolveTimeValue d/w resolution, multi-unit and fractional forms, and invalid input rejection. Existing tests updated to assert resolveTimeValue error returns before checking output.
Query builder integration and validation
internal/query/builder.go, internal/query/builder_test.go
buildWhere now calls resolveTimeValue with error propagation for timeRange.since, and TestBuild_InvalidColumns validates the build fails with "invalid time value" when since is unparseable.
Documentation and changelog
CHANGELOG.md, docs/src/content/docs/api.md, docs/src/content/docs/sdk.md, internal/query/ast.go
Adds Unreleased "Fixed" entry documenting day/week suffix support and closed-fail error handling. API and SDK docs updated to list 7d/2w as supported relative formats and mention 400 errors for unparseable time_range values. Minor inline comment formatting updated in TimeRange.Since.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes


Note

🎁 Summarized by CodeRabbit Free

Your organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login.

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added documentation Improvements or additions to documentation go Pull requests that update go code area/query Structured query AST, SQL builder area/docs Documentation, site/, README labels Jun 6, 2026
@github-actions
github-actions Bot requested a review from EricAndrechek June 6, 2026 17:20
@taitelee taitelee moved this from Backlog to Ready in WaveHouse Task Board Jun 6, 2026
@github-actions

github-actions Bot commented Jun 6, 2026 •

Copy link
Copy Markdown

📚 Docs preview is live: https://12e06032-wavehouse-docs.wave-rf.workers.dev

Updated for commit 365232d

@github-project-automation github-project-automation Bot moved this from Ready to In progress in WaveHouse Task Board Jun 8, 2026
@EricAndrechek
EricAndrechek merged commit cfb43e4 into main Jun 8, 2026
7 checks passed
@EricAndrechek
EricAndrechek deleted the bug/time_range-RFC3339 branch June 8, 2026 12:12
@github-project-automation github-project-automation Bot moved this from In progress to Done in WaveHouse Task Board Jun 8, 2026
EricAndrechek added a commit that referenced this pull request Jun 8, 2026
Merging origin/main (#292) closed #285 — `7d`/`2w` now resolve to hours
in the structured-query builder, so two rationale comments that justified
"168h, not 7d, because #285" are now false:

- LiveDemo.astro:259 — fallback comment's `("168h" not "7d" — see #285.)`
- CHANGELOG.md — the live-demo entry's `(168h, since day-suffixed
  durations are #285)`

The `168h` literal in the fallback query is unchanged (an explicit hours
value is still perfectly valid); only the obsolete justification is removed.
Flagged by the pre-push code review after the merge.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docs Documentation, site/, README area/query Structured query AST, SQL builder documentation Improvements or additions to documentation go Pull requests that update go code

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

bug(query): day-suffix relative durations in time_range ("7d") reach ClickHouse raw and error — sdk.md promises them

2 participants