feat: Implement date_part scalar function - #27005
Conversation
Remove a lot of code that wasn't needed for date_part including iterator creation. We can just map values similar to simple math functions.
|
|
||
| const ( | ||
| DatePartString = "date_part" | ||
| DatePartTimeString = "date_part_time" |
There was a problem hiding this comment.
What is date_part_time? I don't see any tests for it.
There was a problem hiding this comment.
It's used to create a reference to time since time is an auxiliary field https://github.com/influxdata/influxdb/pull/27005/files#diff-609a7e16be956ed6386e1a4a4efadf600b7d4de7dcfea27330dc692d1e901dc8R930-R944 I'm going to create some ValueMapper tests for this.
There was a problem hiding this comment.
@gwossum I can add tests for this but it would likely require exporting
Line 881 in 362217b
davidby-influx
left a comment
There was a problem hiding this comment.
Some changes from the first pass. Will review again after changes.
davidby-influx
left a comment
There was a problem hiding this comment.
query/subquery.go:133— user-triggerable process panic / DoS · CONFIRMED
mapAuxField now returns a datePartMap for any GROUP BY date_part dimension name, including the subquery's aggregate driver. But NewIteratorMapper's driver switch only handles FieldMap/TagMap and panics on the default case.
- Trigger:
SELECT count(year) FROM (SELECT year FROM cpu) GROUP BY date_part('year', time)— a subquery whose aggregate argument is named after a date part. - Impact:
panic: unable to create iterator mapper with driver expression type: query.datePartMap(iterator_mapper.go:84), fired inside an errgroup goroutine spawned bybuildCursor— the handler'srecovercan't catch it, so influxd crashes. Any user who can run queries can trigger it. Reproduced live by the verifier.
query/date_part.go:429— negative epoch groups sort out of order · CONFIRMED
computeDimKey encodes the signed int64 as unsigned big-endian: binary.BigEndian.PutUint64(buf[:], uint64(val)). A negative int64 becomes a huge uint64, so its bytes sort after every non-negative value, and reduce() sorts these DimKey strings to order the emitted rows.
- Trigger:
date_part('epoch', time)over data spanning 1970 (InfluxDB ns timestamps reach back to 1677), e.g.SELECT count(value) FROM cpu GROUP BY date_part('epoch', time). - Impact: pre-1970 (negative) buckets are emitted at the wrong end instead of chronologically. Silent for all other parts (year/month/dow/hour are non-negative), which is why the positive-only tests pass.
query/iterator.go:1051— rolling-upgrade mis-grouping across the wire codec · PLAUSIBLE
encode/decodeIteratorOptions add proto fields DatePartDimensions (23) and NeedTimeRef (24). An older remote data node drops the unknown fields, so it never computes the date_part aux value.
- Impact: during a mixed-version rolling upgrade (new coordinator, old data node), a distributed
GROUP BY date_part(...)query returns points with no date_part aux;len(aux) < len(dims)collapses everything into one ungrouped series — silently wrong results, no error. Verifier confirmed the mechanism but couldn't fully exercise the cross-version path, hence PLAUSIBLE.
query/compile.go:975— observable error-text change · CONFIRMED (low impact)
The invalid-GROUP-BY-function error was reworded from "only time() calls allowed in dimensions" to "only time() and date_part() calls allowed in dimensions". Any client/driver/test that string-matches the old text (e.g. for GROUP BY now()) silently stops matching. Arguably a correct improvement — flagged only because it's an observable behavior change.
Cleanups
query/iterator.go:722—DatePartDimension.Nameis redundant derivable state · CONFIRMED
Name is always Expr.String(), yet it's round-tripped over the wire and read at some sites (select.go:686/719, subquery.go:64) while cursor.go:279 ignores it and recomputes Expr.String(). It's self-documented as a footgun: a decoded/hand-built dimension whose Name diverges silently yields an unpopulated grouped column. Drop the field; use Expr.String() everywhere.
query/select.go:710— twoif len(opt.DatePartDimensions) > 0loops over the same slice · CONFIRMED
Lines 682–691 and 710–721 both guard on the same condition and iterate the same dimensions (one appends the Field, the other the Aux/auxKeys). Easy to drift — add a dim to one loop but not the other and you get a column with no backing aux slot. Merge into one pass.
query/cursor.go:487—newFilterCursordefeats theNeedTimeRefcache · CONFIRMED
opt.NeedTimeRef is computed once and documented as caching the AST walk "to avoid repeatedly walking the condition AST for every iterator creation" — but newFilterCursor calls conditionNeedsTimeRef(filter) again per subquery filter cursor, re-walking the whole tree. Pass opt.NeedTimeRef (or opt) in.
query/cursor.go:165— duplicated "expr contains a date_part Call" WalkFunc · PLAUSIBLE
The same WalkFunc{ if call.Name == DatePartString ... } idiom appears in scannerCursorNeedsDatePart (cursor.go:171), conditionNeedsTimeRef (iterator.go:755), and inline in compile.go (validateDatePartSelectFields, validateDatePartAnchor). Change how date_part is identified and you must update every copy; miss one → inconsistent detection. Hoist exprContainsDatePart(expr).
models/rows.go:36(and:19) — FNV-hashing an already-sorted slice to compare it · CONFIRMED
SameSeries computes a full FNV64a hash of both GroupingKeys slices on every per-row comparison (chunked responses in handler.go:874), even though the slices are already sorted (emitter.sortedKeys). cmd/influx/cli/cli.go:884 does the same comparison with slices.Equal. The hash is more work, inconsistent with the CLI, and two distinct key sets that collide under FNV64a would merge into one series. Use slices.Equal.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 33 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- query/internal/internal.pb.go: Generated file
Comments suppressed due to low confidence (2)
query/emitter.go:80
Emitterstoresrow.GroupingKeysdirectly ine.groupingKeys, but that map is reused and cleared by cursor scanning (scannerCursorBase.Scancallsclear(row.GroupingKeys)). This aliases mutable state across iterations and can cause grouping keys to change while the emitter is still coalescing rows, splitting/merging series incorrectly. Clone the map (or store an immutable representation) before saving it on the emitter.
func (e *Emitter) createRow(series Series, groupingKeys map[string]struct{}, values []interface{}) {
e.series = series
e.groupingKeys = groupingKeys
e.row = &models.Row{
Name: series.Name,
query/iterator.gen.go.tmpl:569
- When an aux value is
nilanddefaultValue == SkipDefault,ScanAtcurrently leavesm[k.Val]unchanged. With date_part grouping, reduce intentionally sets non-active date_part aux slots tonilso those dimension column values should become null; leaving the map entry untouched can leak a prior row's value into the current row. Clear the map entry when no default fill is applied.
// Insert the fill value if one was specified.
if s.defaultValue != SkipDefault {
m[k.Val] = castToType(s.defaultValue, k.Type)
}
}
|
The first suppressed copilot finding above does point to a future-proofing we might want to do:
|
davidby-influx
left a comment
There was a problem hiding this comment.
We need tests for date_part roll-up across dst boundaries in both directions:
- A date_part('day') roll-up where a single local day is 23 or 25 hours long and holds points on both sides of the transition.
- GROUP BY date_part('hour', time) under tz() at fall-back, where local hour 1 occurs twice, the case where two distinct UTC instants must merge into one bucket.
- Spring-forward's non-existent local hour (02:00–03:00) as a grouping key or a WHERE date_part('hour', time) = 2 filter.
- Southern-hemisphere / non-US transition rules, or half-hour-offset zones (e.g. Australia/Lord_Howe).
I've added tests for DST and rollups 👍 |
davidby-influx
left a comment
There was a problem hiding this comment.
Some more suggested tests, one optimization, a defensive change to the Emitter
davidby-influx
left a comment
There was a problem hiding this comment.
One code nit, but needs spring forward tests, as well
only occurs on date_part queries to leave previous code un-changed
use slices.ContainsFunc to check duped date_part dims
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 34 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- query/internal/internal.pb.go: Generated file
Comments suppressed due to low confidence (1)
query/point.go:295
decodeAuxassumes the date_part grouping key is stored as a raw 9-byteStringValueand passes it directly todecodeKey. If the encoding is changed to be protobuf-safe (see comment above), this decode path must be updated in lockstep (including validation/error handling) so date_part grouping still round-trips over the iterator wire codec.
case auxDatePartKey:
if pb[i].StringValue != nil {
if key, err := decodeKey(*pb[i].StringValue); err == nil {
aux[i] = key
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 35 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- query/internal/internal.pb.go: Generated file
Comments suppressed due to low confidence (1)
query/iterator.gen.go.tmpl:568
- In IteratorScanner.ScanAt, when an aux value is nil (or otherwise hits the default case) and defaultValue is SkipDefault, the map entry for that key is left untouched. With date_part grouping, non-active dimension aux slots are intentionally nil, so this can leave a stale value from the previous point in the evaluation map and leak into result columns / expression evaluation.
Consider clearing the key when no default is applied (e.g. delete(m, k.Val)) so nil aux values reliably produce NULL for that column.
default:
// Insert the fill value if one was specified.
if s.defaultValue != SkipDefault {
m[k.Val] = castToType(s.defaultValue, k.Type)
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 35 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- query/internal/internal.pb.go: Generated file
Suppressed comments (1)
query/iterator.go:1110
decodeIteratorOptionsdecodesDatePartDimensionsby casting the on-wireExprint directly toDatePartExprwithout range validation. If an iterator options payload is received with an unknown/invalid expr (e.g. mixed-version cluster, corruption), this can lead to silent wrong results in some paths (e.g.datePartMapused as an iterator-mapper driver will treat an invalid expr as a nil value and some aggregates may quietly return 0 instead of failing fast).
Consider validating d.GetExpr() here (must be Year <= expr < Invalid) and returning an error if out of range before constructing the DimensionGrouper. This makes the system fail loudly instead of producing misleading output.
// Decode date_part GROUP BY dimensions and rebuild the grouper from them.
if dims := pb.GetDatePartDimensions(); len(dims) > 0 {
opt.DatePartDimensions = make([]DatePartDimension, len(dims))
for i, d := range dims {
opt.DatePartDimensions[i] = DatePartDimension{
Expr: DatePartExpr(d.GetExpr()),
}
}
opt.DimensionGrouper = NewDatePartGrouper(opt.DatePartDimensions)
Implements
date_part(part, expression), which extracts a component from a timestamp.Signature:
date_part('<part>', time)timeVarRef, nothing elseint64, evaluated in the query timezone (tz(...)), default UTCParts:
yearquarter[1, 4]month[1, 12]week[1, 53]day[1, 31]hour/minute/second[0,23]/[0,59]/[0,59]millisecond/microsecond/nanosecond45.123sreturns45123formilliseconddowisodowdoy[1, 366]epochweekis the ISO week andyearis the calendar year, so the two can disagree atyear boundaries. For example 2023-01-01 returns week 52.
Examples:
SELECT rules
date_partaggregate or selector.
date_part-only selects are rejected.date_partfields and aliases are allowed, and may nest in expressionssuch as
date_part('hour', time) + 1.GROUP BY date_part rules
time(). Other calls in GROUP BY are rejected.date_partaggregate or selector in the SELECT list.Raw selects and multiple aggregates are rejected.
date_part('part', time)must match a grouped part. A non-groupedpart is rejected because it is undefined for the bucket. A non-active grouped
part yields null in that series.
year. A field or aliascolliding with it is rejected.
fill(none)is always supported.fill(null)(the default) is supported for abare GROUP BY date_part but rejected when combined with a
time()interval; usefill(none).fill(previous),fill(linear), andfill(<value>)are rejected.Subqueries
date_partis supported in the WHERE clause and in GROUP BY of a query over asubquery source, computed from the timestamps the subquery emits.
tz(...)ishonored.
a stored field or a non-
date_partaggregate or selector.See #27001 for 1.x limitations.