feat(sql): support date types and calendar interval expressions - #419
Merged
Merged
Conversation
…as DataType and support Interval as literal and DataType
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved date/interval handling and data-quality coverage issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Expands SQL and pre-ASAP support for date/interval types and adds TPC-H data-quality coverage.
Changes:
- Adds date and interval type/scalar handling.
- Updates temporal expression inference and SQL type bridging.
- Adds DQC and workload coverage.
File summaries
| File | Summary | Final review notes |
|---|---|---|
crates/types/src/pre_asap/schema.rs |
Adds Date and Interval types. |
— |
crates/types/src/pre_asap/query_expr.rs |
Adds temporal arithmetic inference and tests. | Negative interval arithmetic can infer Float64 (moderate, 2 votes). |
crates/types/src/pre_asap/expr_ir.rs |
Adds interval scalar representation. | — |
crates/frontend-sql/tests/data_quality_check/tpch_deequ.rs |
Adds TPC-H lowering coverage. | Exercise date columns directly and assert the exact rejected query set (moderate, 2 votes; moderate, 1 vote). |
crates/frontend-sql/tests/data_quality_check/data/tpch_deequ_queries.sql |
Adds the DQC query corpus. | Composite COUNT(DISTINCT ...) may count only the first column (critical, 1 vote). |
crates/frontend-sql/tests/bgp_jan2024_workload/bgp_jan2024_workload.rs |
Updates interval-related tally expectations. | Update the stale explanatory rationale (nit, 1 vote). |
crates/frontend-sql/src/sql/types.rs |
Bridges date and interval SQL types. | Date literals lack lowering, and reverse interval mapping is missing (moderate, 3 votes; moderate, 1 vote). |
crates/frontend-sql/Cargo.toml |
Registers the integration test. | — |
Review details
Suppressed comments (3)
crates/frontend-sql/src/sql/types.rs:187
dtype_to_arrowemits an Arrow interval forDataType::Interval, but the reverse match still rejects everyArrowDataType::Interval. The comment above explicitly acknowledges that a hand-built catalog can reach this branch, so such a schema cannot round-trip and interval-typed planning paths fail withUnsupportedFeature. Add the reverse interval mapping (or rejectIntervalbefore registration) instead of exposing a one-way bridge.
ArrowDataType::Interval(datafusion::arrow::datatypes::IntervalUnit::MonthDayNano)
crates/frontend-sql/tests/bgp_jan2024_workload/bgp_jan2024_workload.rs:204
- The tally update is inconsistent with the explanatory paragraph immediately above it: that paragraph still says the two
toStartOfIntervalqueries fail inOtherbecause interval conversion is unsupported, while these new expectations say both lower andOtheris zero. Please update that rationale as part of this tally change so the pinned test does not document the old failure mode.
// 152 -> 154: `ScalarValue::Interval` (this branch) converts the
// `INTERVAL x unit` literal the two `toStartOfInterval(...)` queries
// carry, which the note above recorded as an open companion gap.
expect(Category::Lowered, 154);
crates/frontend-sql/tests/data_quality_check/tpch_deequ.rs:116
- This ratchet checks only the aggregate
lowered == 48, so it can pass if one currently supported query starts rejecting while one of the twocorrqueries starts lowering (or the reverse). Since the comment promises that the two rejects are specificallycorr, track the query IDs/error categories and assert that exact rejected set so the test detects coverage swaps, not just the total.
// Coverage ratchet. The 2 that do not lower are the `corr` cells; adding a
// binary `AggIntent` would raise this to 50.
assert_eq!(
t.lowered, 48,
"SQL lowering coverage moved off the DQC ratchet: {t:?}"
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
zzylol
approved these changes
Sep 15, 2026
Selvomega
added a commit
that referenced
this pull request
Sep 15, 2026
Two conflicts, both from main landing after this branch was cut. `tests/sql_lowering.rs`: each side appended a test function at the same place — `corr_result_is_nullable_float` here, `composite_distinct_is_rejected` on main (#419). They are unrelated, so both are kept. `tests/data_quality_check/tpch_deequ.rs`: no textual conflict, since this branch never touched the file. #419 added the corpus ratchet and pinned U-P4d and U-P4l as rejected-because-`corr`. Both lower now, so the ratchet moves: 47 -> 49 lowered, P2b alone rejected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Selvomega
added a commit
that referenced
this pull request
Sep 16, 2026
…UNT(DISTINCT) Two changes, in dependency order. The second is only expressible because of the first. `AggIntent` asserted that an aggregate reads one column: `input_col() -> Option<ColumnId>` was how every consumer reached an input. #421 added `PearsonCorr { left, right }` and had to make it return `None` from that accessor *defensively*, so no single-column consumer could pick up half of the pair. The accessor had become a trap each new multi-column intent must remember to opt out of, and the cost of forgetting is a silently wrong number rather than a compile error. That is also why `COUNT(DISTINCT a, b)` had nowhere to land. It counts distinct *tuples*; SQL lowering dropped every argument after the first, reporting single-column cardinality where the query asked for tuple cardinality. #419 stopped the miscount by rejecting the call outright, which left TPC-H/Deequ U-P2b and nine synthetic-packet-trace flow counts permanently unlowerable. Before this PR: SELECT count(DISTINCT l_orderkey, l_linenumber) * 1.0 / count(*) FROM lineitem -> lowering failed: unsupported aggregate: multi-column COUNT(DISTINCT) After this PR: Aggregate { measures: [Cardinality { cols: [0, 1], .. }], .. } What changed: - `input_col()` is removed. `input_cols() -> Vec<C>` is the only column accessor, so no consumer can ask for "the" input column of an aggregate that reads two. An intent may now declare whatever arity its semantics need without a defensive opt-out, and `PearsonCorr`'s opt-out is gone. The genuinely unary reducers keep `col: Option<C>` — this relaxes what consumers assume, it does not widen every variant. - `AggIntent::Cardinality` takes `cols: Vec<C>`. One entry is `COUNT(DISTINCT col)`, several count distinct tuples, empty keeps the PromQL "the sample value" convention. It is the first variable-arity intent, and was not expressible while the accessor split existed. - Realization is the single-column one with a wider item: a tuple becomes a `SummaryInputExpr::Tuple`, which HLL/Theta/KMV hash as one value. UnivMon is withheld from a tuple — it estimates frequency moments over a single value stream — and the value-frequency rule refuses a multi-column intent directly, so that invariant does not rest on the candidate table alone. - SQL lowering resolves every `COUNT(DISTINCT ...)` argument as a grouping key, so a qualifier survives a join; an expression argument is rejected rather than reduced over a probe column. Serialization of `Cardinality` changes from `col` to `cols`; a payload that omits the field still reads as the implicit input. Both corpus ratchets move in this commit: tpch_deequ 49 -> 50 lowered with no rejections, synthetic_packet_trace 61 -> 70. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
SQL data-quality checks using
CAST(... AS DATE)orINTERVAL '30' DAYfailed during lowering because the canonical IR lacked date and calendar-interval types.What and how
Add
DataType::Date,DataType::Interval, and a calendar-interval literal carrying months, days, and nanoseconds. Bridge Arrow date and interval types into these representations and preserve temporal result types through date/timestamp shifts and interval negation. Normalize typed Arrow date literals to date casts and explicitly reject multi-columnCOUNT(DISTINCT ...), whose tuple semantics the IR cannot represent. Reject fixed-duration subtraction before lowering, including inside predicates, CASE branches, and sort expressions, because the IR cannot preserve its unit.Before this PR
l_receiptdate <= l_shipdate + INTERVAL '30' DAYfailed with an unsupported interval scalar;CAST('1992-01-01' AS DATE)failed with unsupported ArrowDate32.After this PR
Both expressions lower successfully. Typed Arrow date literals also lower as date casts. Date shifts retain
Date, timestamp shifts retainTimestamp, and-CAST('1 day' AS INTERVAL)retainsInterval. Unsupportedd - dduration results fail explicitly even in expressions such as(d - d) IS NULL.Verification
Float64; both pass after the fixes.Datecolumns and lowers 47 of 50 checks, with the exact rejected IDs and reasons pinned; the BGP corpus now lowers 154 queries, up from 152.cargo +1.98.0 test --workspace --locked, workspace Clippy with warnings denied, and formatting checks pass.cargo +stable, whose installed toolchain is missingrustc.Limitations
Arrow
Date64normalizes toDateand registers back asDate32. Fixed-duration results remain unsupported. Composite distinct counts and the two TPC-H correlation checks remain unsupported. Verification covers planning/lowering, not database execution; performance measurements and screenshots are not applicable.Closes #416
Closes #418