Repository navigation
fix(promql): read sample values in date functions with an argument - #9484
fengyue-xve wants to merge 4 commits into
Conversation
PromQL date functions with an argument (minute, hour, month, year, day_of_month, day_of_week, day_of_year and days_in_month) treat the sample value as a Unix timestamp in seconds, truncated towards zero and interpreted as UTC, not as the time index of the sample. Calls without an argument keep reading the evaluation instant, which the time index carries. Native histogram samples are skipped, matching Prometheus: when the table materializes float alternative samples, the float column carries the value, otherwise the column evaluates to NULL and the row is dropped. Sqlness expectations are updated for the changed column names, and a regression case covers sample values whose calendar components differ from the evaluation instants. Fixes GreptimeTeam#9393 Signed-off-by: fengyue-xve <1759519137@qq.com>
killme2008
left a comment
There was a problem hiding this comment.
Skipping native histogram samples is new behavior and nothing pins it: removing the field_column_is_native_histogram branch keeps every test green. sqlness cannot ingest histograms, so a planner test along the lines of test_mixed_or_routes_float_histogram_and_label_functions would cover it.
| let timestamp_expr = DfExpr::Cast(Cast::new( | ||
| Box::new(DfExpr::Column(Column::from_name(value))), | ||
| ArrowDataType::Timestamp(ArrowTimeUnit::Second, None), | ||
| )); |
There was a problem hiding this comment.
Cast runs with safe: false, so a NaN or ±Inf sample fails the whole query ("Can't cast value NaN to type Int64"), e.g. hour(foo / bar) with a zero bar. Before this change these functions never read the value, and Prometheus still emits a point for it. Use TryCast so the value becomes NULL and the row is dropped by the empty-value filter. days_in_month has the same problem past ~9.2e9 s: date_trunc scales seconds to nanoseconds and errors with "Timestamp out of range".
There was a problem hiding this comment.
My TryCast suggestion is incomplete: it drops non-finite samples, while Prometheus still emits them, and it does not prevent the later date_trunc overflow. Cover non-finite inputs and year-3000 sample values against Prometheus before choosing the conversion.
There was a problem hiding this comment.
Both points were right — I reworked the conversion around what Prometheus actually emits.
The sample is now read as coalesce(TRY_CAST(v AS Timestamp(Second)), epoch), guarded by an IS NOT NULL case: a non-finite or out-of-i64-range value becomes the Unix epoch rather than NULL, so the row survives and a point is emitted. That harmonizes with Prometheus, where the value goes through int64(v): Go leaves the overflow cases implementation-defined (golang/go#76264) and int64(NaN) is 0 on arm64, so the epoch is the portable result. NULL stays NULL — a skipped native histogram column has no sample — and keeps dropping the row instead of turning it into a 1970 point.
days_in_month no longer goes through date_trunc("month", ts), which scaled seconds to nanoseconds and failed past ~2262: it reads date_part("month")/date_part("year") and selects the month length with a proleptic Gregorian leap-year CASE, the same rule as time.Date in Prometheus.
time_fn.sql pins NaN, ±Inf, 1e19 and a year-3000 sample value for all eight functions, and the planner tests pin how the date functions skip native histogram columns.
killme2008
left a comment
There was a problem hiding this comment.
Hi @fengyue-xve, a friendly ping: the open points from the previous review still apply to 07dd1ed. The strict Cast errors on NaN, ±Inf and out-of-range samples (arrow casts numeric → Timestamp through Int64 with safe: false), and the native histogram skip has no test. Please pin the non-finite and year-3000 outputs against Prometheus in time_fn.sql before picking the conversion. Happy to help if anything is unclear.
| let date_part = match func.name { | ||
| "minute" => "minute", | ||
| "hour" => "hour", | ||
| "month" => "month", | ||
| "year" => "year", | ||
| "day_of_month" => "day", | ||
| "day_of_week" => "dow", | ||
| "day_of_year" => "doy", | ||
| // days_in_month | ||
| _ => "day", |
There was a problem hiding this comment.
nit: days_in_month never uses the "day" from the _ arm. Match it explicitly and keep date_part to the seven date_part functions, so a new name added to the outer arm fails to compile instead of silently mapping to "day".
There was a problem hiding this comment.
Done in 48c7e37, and it still holds at the current head: days_in_month has its own arm (with no date_part unit), the inner match holds exactly the seven date_part units and ends in unreachable!, so a new name in the outer arm fails to compile.
`days_in_month` is built by `days_in_month_expr` and never selects a `date_part` unit, so mapping it through a `_ => "day"` arm left a dead value and would silently map any future function added to the arm. Map the seven `date_part` functions to `Some(unit)`, match `days_in_month` explicitly as `None`, and make the remaining arm `unreachable!`. Signed-off-by: 风月 <1759519137@qq.com> Signed-off-by: fengyue-xve <1759519137@qq.com>
A date function with an argument read the sample value with a strict `Cast` to `Timestamp(Second)`, so a NaN, an infinity or a value beyond the `int64` range failed the whole query instead of yielding a point. Prometheus emits a point for such samples: Go leaves `int64(v)` implementation-defined (golang/go#76264 is still open) and converts NaN to 0 on arm64, so harmonize on the Unix epoch through `TRY_CAST` and `coalesce`. A NULL value is not a sample and keeps yielding NULL, which drops the row. `days_in_month` no longer goes through `date_trunc("month", ts)`, which scaled the timestamp to nanoseconds and failed with "Timestamp out of range" beyond ~year 2262. It reads the `date_part` month and year and selects the month length with a proleptic Gregorian leap-year CASE, like `time.Date` in Prometheus. The conversion would otherwise become the output column name, so the expressions are aliased to `<function>(<column>)`. Pin the new behavior: - planner tests: the eight date functions drop native histogram samples when they are the only field columns, and skip the histogram column when an alternative float column carries the same sample - `time_fn.sql` and `time_fn.result`: non-finite, out-of-range and year-3000 sample values for every date function Signed-off-by: 风月 <1759519137@qq.com> Signed-off-by: fengyue-xve <1759519137@qq.com>
killme2008
left a comment
There was a problem hiding this comment.
Please merge the latest main into this branch to catch up with the current codebase, then rerun the relevant PromQL planner tests and sqlness cases.
| DfExpr::TryCast(TryCast::new( | ||
| Box::new(column), | ||
| ArrowDataType::Timestamp(ArrowTimeUnit::Second, None), | ||
| )), | ||
| DfExpr::Literal(ScalarValue::TimestampSecond(Some(0), None), None), |
There was a problem hiding this comment.
TRY_CAST accepts finite values such as 1e13, but Arrow's date_part returns NULL outside Chrono's calendar range. The empty-value filter then drops the sample, whereas Prometheus returns year 318857 and minute 46. Handle this extraction boundary without silently losing samples, and add a regression for 1e13.
…ample-value Signed-off-by: fengyue-xve <1759519137@qq.com>
|
Done in |
What's changed
PromQL date functions that take an argument —
minute,hour,month,year,day_of_month,day_of_week,day_of_yearanddays_in_month— should read each sample value as a Unix timestamp in seconds (UTC), but the planner applied them to the sample's time index instead, so every series reported the calendar component of the evaluation instant:The fix reworks the date-function arms of
create_function_expr:Timestamp(Second)— the float → int conversion truncates towards zero — anddate_part(or thedays_in_monthexpression) is computed from the sample value. Non-finite and out-of-i64-range values (NaN, ±Inf, 1e19) resolve to the Unix epoch instead of failing the query, harmonizing with Prometheus'int64(sample), whose overflow cases are implementation-defined in Go (proposal: cmd/compile: floating point to integer conversions should saturate on overflow golang/go#76264; 0 on arm64). Each expression is aliased to<function>(<column>)so the conversion does not become the output column name.days_in_monthis factored into a shareddays_in_month_exprhelper so both paths build the same expression; it selects the month length fromdate_part("month")/date_part("year")with a proleptic Gregorian leap-year CASE instead ofdate_trunc("month", ts), which scaled seconds to nanoseconds and overflowed past ~year 2262.Tests
Updated the sqlness expectations for the changed output column names (
time_fn.result,dotted_column_names.result) and added a regression case intime_fn.sqlwith ten sample values covering all eight functions: calendar components that deliberately differ from the evaluation instants, a fractional value (59.9→59), a negative one (-1.5→ 1969-12-31T23:59:59Z), and non-finite, out-of-range and year-3000 values (NaN,inf,-inf,1e19,32503680000), which must yield a point (the Unix epoch) instead of failing the query. Planner tests pin that the date functions drop native histogram samples when they are the only field columns, and skip the histogram column when a float alternative carries the same sample. The existing no-argument cases are kept to pin the default behavior.Fixes #9393