-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix(tesseract): honour custom granularity origin for single-unit intervals #11432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,42 @@ impl QueryDateTime { | |
| ) | ||
| } | ||
|
|
||
| /// Whether this instant sits exactly on the start of the given predefined granularity — | ||
| /// i.e. whether `DATE_TRUNC(granularity, self)` would leave it unchanged. | ||
| /// | ||
| /// Judged on the local wall clock, so a midnight that a DST gap pushed forward to 01:00 still | ||
| /// counts as the start of its day. | ||
| pub fn is_start_of(&self, granularity: &str) -> Result<bool, CubeError> { | ||
| let dt = self.naive_local(); | ||
| // Midnight can be absent from the local calendar when a DST gap swallows it; the day then | ||
| // starts at whatever instant the gap resolves to, and that is still its start. | ||
| let midnight = dt.date().and_hms_opt(0, 0, 0).ok_or_else(|| { | ||
| CubeError::internal(format!("Failed to build midnight for {}", dt.date())) | ||
| })?; | ||
| let start_of_day = | ||
| Self::from_local_date_time(self.date_time.timezone(), midnight)?.naive_local(); | ||
| let starts_the_day = dt.time() == start_of_day.time() && dt.date() == start_of_day.date(); | ||
| let zero_time = starts_the_day && dt.nanosecond() == 0; | ||
|
|
||
| let res = match granularity { | ||
| "year" => zero_time && dt.month() == 1 && dt.day() == 1, | ||
| "quarter" => zero_time && dt.day() == 1 && (dt.month() - 1).is_multiple_of(3), | ||
| "month" => zero_time && dt.day() == 1, | ||
| "week" => zero_time && dt.weekday() == Weekday::Mon, | ||
| "day" => zero_time, | ||
| "hour" => dt.minute() == 0 && dt.second() == 0 && dt.nanosecond() == 0, | ||
| "minute" => dt.second() == 0 && dt.nanosecond() == 0, | ||
| "second" => dt.nanosecond() == 0, | ||
| other => { | ||
| return Err(CubeError::internal(format!( | ||
| "Unexpected granularity '{other}' in date alignment check" | ||
| ))) | ||
| } | ||
| }; | ||
|
Comment on lines
+74
to
+88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This match is a near-duplicate of The two now co-decide the same grain: for the Asuncion-style origin from your test,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair, and the inconsistency you name is real: |
||
|
|
||
| Ok(res) | ||
| } | ||
|
|
||
| pub fn date_time(&self) -> DateTime<Tz> { | ||
| self.date_time | ||
| } | ||
|
|
@@ -205,6 +241,29 @@ impl QueryDateTime { | |
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn is_start_of_accepts_a_midnight_swallowed_by_a_dst_gap() { | ||
| // Paraguay springs forward at midnight, so 2023-10-01T00:00 does not exist locally and | ||
| // resolves to 01:00. It is still the first instant of that day, month and quarter. | ||
| let tz = "America/Asuncion".parse::<Tz>().unwrap(); | ||
| let d = QueryDateTime::from_date_str(tz, "2023-10-01").unwrap(); | ||
|
|
||
| assert_eq!(d.default_format(), "2023-10-01T01:00:00.000"); | ||
| assert!(d.is_start_of("day").unwrap()); | ||
| assert!(d.is_start_of("month").unwrap()); | ||
| assert!(d.is_start_of("quarter").unwrap()); | ||
| assert!(!d.is_start_of("year").unwrap()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn is_start_of_still_rejects_a_real_time_component() { | ||
| let tz = "America/Asuncion".parse::<Tz>().unwrap(); | ||
| // 2023-10-02 has an ordinary midnight, so 01:00 on it is genuinely mid-day. | ||
| let d = QueryDateTime::from_date_str(tz, "2023-10-02T01:00:00").unwrap(); | ||
| assert!(!d.is_start_of("day").unwrap()); | ||
| assert!(d.is_start_of("hour").unwrap()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_date_time() { | ||
| let tz = "Etc/GMT-3".parse::<Tz>().unwrap(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,6 +167,100 @@ async fn test_type_time_alias_wraps_compound_exprs_before_tz_cast() { | |
| } | ||
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn test_fiscal_year_by_origin_does_not_truncate_to_calendar_year() { | ||
| let ctx = create_context(); | ||
|
|
||
| let query = indoc! {" | ||
| measures: | ||
| - orders.count | ||
| time_dimensions: | ||
| - dimension: orders.created_at | ||
| granularity: fiscal_year_by_1st_april | ||
| dateRange: | ||
| - \"2024-04-01\" | ||
| - \"2026-03-31\" | ||
| order: | ||
| - id: orders.created_at | ||
| "}; | ||
|
|
||
| let sql = ctx.build_sql(query).unwrap(); | ||
|
|
||
| // `1 year` is a trivial interval, so the grain used to fall onto the DATE_TRUNC branch, | ||
| // which has no way to express an April 1 origin. | ||
| assert!( | ||
| !sql.to_lowercase().contains("date_trunc('year'"), | ||
| "off-boundary origin was truncated to the calendar year\nFull SQL:\n{sql}" | ||
| ); | ||
|
|
||
| if let Some(result) = ctx.try_execute_pg(query, SEED).await { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The No That matters because the
Meanwhile the planner's own bucketing — Running these tests against Postgres and committing the snapshots would either confirm the fiscal-year buckets are right or surface the drift; right now the PR's headline example isn't verified end to end.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, and it's worse than the snapshots being absent. Working it through against the CI run that did execute these ( The fixture rows bucket correctly —
Meanwhile Fixing it properly means giving |
||
| insta::assert_snapshot!(result); | ||
| } | ||
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn test_calendar_year_by_origin_still_truncates() { | ||
| let ctx = create_context(); | ||
|
|
||
| let query = indoc! {" | ||
| measures: | ||
| - orders.count | ||
| time_dimensions: | ||
| - dimension: orders.created_at | ||
| granularity: calendar_year_by_origin | ||
| dateRange: | ||
| - \"2024-01-01\" | ||
| - \"2025-12-31\" | ||
| order: | ||
| - id: orders.created_at | ||
| "}; | ||
|
|
||
| let sql = ctx.build_sql(query).unwrap(); | ||
|
|
||
| // An origin that already sits on the year boundary keeps the cheaper DATE_TRUNC rendering. | ||
| assert!( | ||
| sql.to_lowercase().contains("date_trunc('year'"), | ||
| "on-boundary origin should still truncate\nFull SQL:\n{sql}" | ||
| ); | ||
|
|
||
| if let Some(result) = ctx.try_execute_pg(query, SEED).await { | ||
| insta::assert_snapshot!(result); | ||
| } | ||
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn test_fiscal_year_by_origin_with_time_shift_measure() { | ||
| let ctx = create_context(); | ||
|
|
||
| // The shape from the original report: a time-shift measure alongside the plain one, both | ||
| // grouped by a fiscal-year grain. The shifted leaf must bin on the same origin as the | ||
| // unshifted one, or the two sides join on incompatible buckets. | ||
| let query = indoc! {" | ||
| measures: | ||
| - orders.total_amount | ||
| - orders.total_amount_prior_year | ||
| time_dimensions: | ||
| - dimension: orders.created_at | ||
| granularity: fiscal_year_by_1st_april | ||
| dateRange: | ||
| - \"2024-04-01\" | ||
| - \"2026-03-31\" | ||
| order: | ||
| - id: orders.created_at | ||
| "}; | ||
|
|
||
| let sql = ctx.build_sql(query).unwrap(); | ||
|
|
||
| assert!( | ||
| !sql.to_lowercase().contains("date_trunc('year'"), | ||
| "time-shifted leaf truncated to the calendar year\nFull SQL:\n{sql}" | ||
| ); | ||
|
|
||
| if let Some(result) = ctx.try_execute_pg(query, SEED).await { | ||
| insta::assert_snapshot!(result); | ||
| } | ||
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| async fn test_custom_granularity_with_daterange_filter() { | ||
| let ctx = create_context(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NaiveDate::and_hms_opt(0, 0, 0)never returnsNone— midnight is representable for everyNaiveDate— so this error arm is unreachable.dt.date().and_time(NaiveTime::MIN)gives the same value without theResultplumbing, which also lets the fallible part of the function be justfrom_local_date_time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed,
and_hms_opt(0, 0, 0)can't fail. Leaving it for the same push that deals with thedate_binboundary problem above rather than force-pushing a cosmetic change while that's still open.