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
davidby-influx
left a comment
There was a problem hiding this comment.
Some changes from the first pass. Will review again after changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 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
ScanAt, when an aux value falls into thedefaultbranch anddefaultValue == SkipDefault(e.g.fill(none)), the map entry for that key is left untouched. With the new date_part GROUP BY behavior, non-active dimension aux slots are explicitly set tonil, so this path will leave stale values from a previous row inm[k.Val]and can incorrectly populate non-active date_part dimension columns. Clearing the key when no fill default is configured avoids that leakage.
default:
// Insert the fill value if one was specified.
if s.defaultValue != SkipDefault {
m[k.Val] = castToType(s.defaultValue, k.Type)
}
Code Review —
|
| # | Location | Divergence |
|---|---|---|
| 6 | query/date_part.go:152 |
millisecond/microsecond return only the sub-second part — Postgres returns seconds*1000 + frac (off by up to 59,000 ms). |
| 7 | query/date_part.go:162 |
epoch uses t.Unix(), truncating fractional seconds that SQL epoch keeps. |
| 8 | query/date_part.go:163 |
isodow returns 0–6 (Sun=6) instead of SQL 1–7 (Sun=7) — off by one. date_part_test.go asserts the 0–6 values. |
Worth a deliberate decision: if the intent is SQL compatibility, fix the code and the
tests; if InfluxDB-specific semantics are intended, document it.
🟡 Clustered/distributed only — not reachable in single-node OSS (PLAUSIBLE)
- 9.
query/point.go:266—encodeAux/decodeAuxcan't serialize the
DecodedDatePartKeystruct; over the data-node wire codec the grouped value comes back
null and all buckets collapse into one group. - 10.
query/iterator.gen.go.tmpl:1486— the genericFilterIterator.Nextuses
EvalBoolwith a non-CallValuermap, so adate_part(...)WHERE predicate reaching
it filters out every point (zero rows). No in-repo callers ofNewFilterIterator;
reachability uncertain. - 11.
tsdb/engine/tsm1/iterator.gen.go.tmpl:289—itr.mis allocated only when
Condition != nil, but written wheneverNeedTimeRef. Safe locally (encoder keeps the
invariant), but a wire-decoded options struct withNeedTimeRef=true, Condition=nil
panics on a nil-map write → crashes the query/node.
⚡ Efficiency (CONFIRMED)
12. ⭐ DatePartValuer wired into every tsm1 query, date_part or not
tsdb/engine/tsm1/iterator.gen.go.tmpl:235 — unconditionally adds an extra valuer
indirection (an interface call per VarRef/Call lookup, per scanned point) to all
WHERE-filtered queries — the common path. The scanner cursor already gates the
identical wiring behind needDatePart; opt.NeedTimeRef could gate it here too.
This is a per-point CPU regression across the whole engine, not just date_part queries
— arguably higher priority than its category suggests.
13. ResolveKeys allocates per-point garbage discarded on bucket hit
query/date_part.go:381 — allocates a fresh entries slice + a 9-byte EncodedKey
string per point, but the reduce loop consumes EncodedKey only on bucket creation (map
miss) → K−1 of every K allocations are GC garbage. Return only DimKey, compute
EncodedKey lazily, reuse a scratch buffer.
14. Per-field redundant work in Scan
query/cursor.go:277 — the DatePartDimensionsString lookup, type assertion,
dpd.Expr.String(), and GroupingKeys insert are recomputed per field though invariant
across the field loop; hoist above it.
📋 Convention (CONFIRMED)
15. New date_part tests use raw t.Fatal/t.Error instead of testify
tests/server_test.go:8085 (also 8790, 8863, 9242, 9297) — violates the project's
testify rule. The same functions already use require.NoError(t, err, "init error") for
setup, so it's internally inconsistent too.
Refuted (19, not reported)
Mostly DRY/maintainability "keep-in-sync" observations (parallel DatePartExpr switches,
duplicated AST walkers, LocationOrUTC not reused) where verifiers found no current
observable defect, plus the millisecond/isodow duplicates and a
DatePartValuer{}-in-compileFields "dead code" claim that was refuted (it can fire
with a literal second arg).
Suggested fix order
- 1 (data loss) and 12 (engine-wide perf regression) — highest impact.
- Correctness cluster 2–5.
- Decision on SQL semantics 6–8 (fix code+tests, or document).
- Mechanical cleanups 13–15.
|
AI review not verified by a human. Take with a grain of salt. |
- cleanup tests to use testify only
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Going through this and fixed some of the ones that were valid: There are a few left that I'm investigating: |
|
And after the changes, the only items left |
davidby-influx
left a comment
davidby-influx
left a comment
There was a problem hiding this comment.
A few more comments, but still a partial review
| if cur.needDatePart { | ||
| // Clear date_part state from previous scan so it doesn't leak across rows. | ||
| delete(cur.m, DatePartDimensionsString) | ||
| row.GroupingKeys = nil |
There was a problem hiding this comment.
What about suggestion to use clear?
There was a problem hiding this comment.
on row.GroupingKeys?
There was a problem hiding this comment.
Avoids an alloc, no locking like the memory allocator uses, etc.
| if cur.needDatePart { | ||
| // Clear date_part state from previous scan so it doesn't leak across rows. | ||
| delete(cur.m, DatePartDimensionsString) | ||
| row.GroupingKeys = nil |
There was a problem hiding this comment.
You could move the make of the map here
if row.GroupingKeys == nil {
// Do we have a count of max possible keys from compilation
// that we could pass to make? Not critical if missing, because
// clear() will let the map grow monotonically, unlike setting to nil
row.GroupingKeys = make(map[string]struct{})
} else {
clear(row.GroupingKeys)
}
| cur.Next() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
How about a date_part benchmark. What does a typical use cost?
There was a problem hiding this comment.
I have a few date_part benchmarks now:
- BenchmarkIntegerIterator_Next_Condition (This is without date_part as a "base")
- BenchmarkIntegerIterator_Next_DatePartCondition
- BenchmarkIntegerIterator_Next_DatePartDimension
- BenchmarkFilterCursor_DatePartCondition
Outputs:
BenchmarkIntegerIterator_Next_Condition-20 36202430 33.03 ns/op 0 B/op 0 allocs/op
BenchmarkIntegerIterator_Next_DatePartCondition-20 8578252 139.8 ns/op 0 B/op 0 allocs/op
BenchmarkIntegerIterator_Next_DatePartDimension-20 82078786 14.12 ns/op 0 B/op 0 allocs/op
BenchmarkFilterCursor_DatePartCondition-20 33907940 35.11 ns/op 0 B/op 0 allocs/op
davidby-influx
left a comment
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.
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.