refactor(query): optimize time_slice with constant slice arguments by vbmithr · Pull Request #20727 · databendlabs/databend · GitHub
Skip to content

refactor(query): optimize time_slice with constant slice arguments - #20727

Open
vbmithr wants to merge 2 commits into
databendlabs:mainfrom
vbmithr:refactor-time-slice-fast-path
Open

refactor(query): optimize time_slice with constant slice arguments#20727
vbmithr wants to merge 2 commits into
databendlabs:mainfrom
vbmithr:refactor-time-slice-fast-path

Conversation

@vbmithr

@vbmithr vbmithr commented Oct 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

time_slice rebuilt its slice spec from its arguments on every row: SliceSpec::new uppercased the unit and the start/end flag into two new strings to parse them, and sub-day slices were floored with i128::div_euclid, a software division. The binder always passes the slice length, unit and flag as constants (lower_time_slice_expr), so none of it depends on the row.

  • When the three slice arguments are scalars, the spec is built once and the date or timestamp column is mapped in one loop. Otherwise, including when the spec does not parse, the existing per-row path runs unchanged, so errors are reported on every row as before.
  • The unit and flag are parsed with eq_ignore_ascii_case instead of allocating.
  • A sub-day slice whose width fits i64 is floored in i64. The bounds stay i128, so an end past the i64 range is reported exactly as before, and wider slices keep the i128 path.

The expression test-support parser passed time_slice's unit and start/end in the opposite order to the binder, so time_slice could not be evaluated through it. This fixes the order, which the new bench needs.

Benchmark

cargo bench -p databend-common-functions --bench bench -- time_slice (new divan group): a 100,000-row timestamp column, rows 0.8s apart, UTC, single thread, median of two rounds. main, the first two changes alone, and the full PR built with the same profile and run back to back on an idle machine.

expr main parse + i64 PR
time_slice(ts, 3600, 'second') 6.32 ms 4.28 ms 1.32 ms
time_slice(ts, 15, 'minute') 6.34 ms 4.13 ms 1.32 ms
time_slice(ts, 1, 'hour', 'end') 6.38 ms 4.05 ms 1.32 ms

In SQL, one thread and 100M rows, sum(to_int64(time_slice(to_timestamp(number * 1000), 3600, 'SECOND'))) took 6.9s on 1.2.952-nightly, against 1.1s for the same buckets as integer arithmetic and 0.56s with no bucketing.

Tests

  • Unit Test

  • Logic Test

  • Benchmark Test

  • No Test - Explain why

  • 02_0083_function_time_slice.test, results recorded from 1.2.952-nightly (the per-row path): sub-day slices before the epoch and with mixed-case units, slices wider than i64 microseconds, and out-of-range ends, as scalars and over columns (a timestamp column across 1970, a date column by month and week, a nullable column with a NULL row, a column with one row out of range).

  • issue_20416.test (date and timestamp range bounds, including time_slice).

  • Logic tests: the query/functions directory with --enable_sandbox over the http handler.

  • Existing scalar golden tests unchanged (cargo test -p databend-common-functions --test it scalars::).

  • cargo fmt --check, cargo clippy -p databend-common-functions -p databend-functions-scalar-datetime -p databend-common-expression-test-support --tests --benches -- -D warnings.

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

AI assistance

  • AI usage: An AI coding agent profiled the per-row cost, drafted the change and its tests, and ran the benchmarks and local validation.
  • Responsible human: @vbmithr
  • The responsible human has read every line of this diff and can explain each change

🤖 Generated with Claude Code

https://claude.ai/code/session_01LxafFsFHwSmtUCVuLehsjK

This change is Reviewable

@vbmithr @claude
refactor(query): optimize time_slice with constant slice arguments
1ab144a 
time_slice rebuilt its slice spec on every row: SliceSpec::new uppercased
the unit and start/end flag into new strings, and sub-day slices were
floored with i128 division, a software routine. The binder always passes
the slice as constants, so the spec is now built once per column when its
arguments are scalars, the per-row path staying for everything else; the
unit and flag are parsed without allocating, and a slice whose width fits
i64 is floored in i64. 100,000 timestamps: 6.3 ms -> 1.3 ms.

The expression test-support parser passed time_slice's unit and start/end
in the opposite order to the binder; fixed for the new bench.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LxafFsFHwSmtUCVuLehsjK
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T15:34:29.917535Z 1ab144a PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added the pr-refactor this PR changes the code base without new features or bugfix label Oct 16, 2026
@sundy-li sundy-li mentioned this pull request Oct 16, 2026
Open
TCeason
TCeason reviewed Oct 19, 2026
View reviewed changes
Comment thread src/query/functions/src/scalars/timestamp/src/date_time_slice.rs
match time_slice_timestamp(*ts, spec, &tz) {
Ok(value) => output.push(value),
Err(error) => {
ctx.set_error(row, error);

@TCeason TCeason Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking testing suggestion: could we add a case where an out-of-range result occurs only in an unselected IF branch? The new tests cover nullable inputs and out-of-range errors separately, but this combination would help verify that the column fast path preserves error masking through ctx.validity.

For example:

  SELECT if(
      number = 0,
      NULL,
      time_slice(
          to_timestamp(number::int64 - 1),
          9223372036854775807,
          'second'
      )
  )
  FROM numbers(2);

This should return NULL and the Unix epoch without raising an error for the masked-out row. The implementation looks correct to me; this would just lock in that behavior.

@github-actions

github-actions Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

⏱ CI timing · Dev

run 38924569819 · attempt 2 (wall time includes re-run gap) · 33.6 min (main p50 34.8 min, −1.2 min)

stage this run main p50 Δ
total 33.6 min 34.8 min −1.2 min
build (aarch64, ci profile) 23.8 min 22.2 min +1.6 min
├ deps + crates (parallel) 10.8 min — —
├ databend-query codegen 3.4 min — —
├ databend-binaries link 7.7 min — —
└ sccache hit rate 90.0 % — —
test_unit 17.2 min 16.9 min +0.3 min
mac_check — 13.2 min —
tests after build (49 jobs) 9.3 min 11.1 min −1.8 min
├ queue p50 / max 1.5 min / 3.6 min 2.3 min / 5.0 min −0.8 min
├ sqllogic standalone (8 jobs, max) 5.1 min 4.3 min +0.8 min
├ sqllogic cluster (8 jobs, max) 7.7 min 7.1 min +0.7 min
├ sqllogic stage (2 jobs, max) 5.7 min 5.3 min +0.4 min
├ sqllogic other (10 jobs, max) 6.5 min 5.9 min +0.6 min
├ stateless (2 jobs, max) 7.0 min 6.2 min +0.8 min
├ stateful (4 jobs, max) 5.1 min 4.8 min +0.3 min
├ compat (6 jobs, max) 4.9 min 5.0 min −0.2 min
├ meta / logs / lineage / udf (5 jobs, max) 4.5 min 4.5 min −0.0 min
├ ee (2 jobs, max) 1.1 min 1.3 min −0.2 min
├ private_tasks (1 jobs, max) 8.8 min 8.8 min +0.0 min
└ fuzz (1 jobs, max) 1.3 min 1.1 min +0.1 min

Minutes unless stated; durations exclude queue time unless stated; "max" = longest job in the group. Baseline = median of the last 10 successful runs (Dev: Main on main; Cloud: recent Cloud runs). Build breakdown has no baseline. Updated on each push.

@TCeason
TCeason enabled auto-merge October 20, 2026 01:17
@TCeason

TCeason commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

@vbmithr @claude
test(query): time_slice error masked out of an IF branch
60c0170 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
auto-merge was automatically disabled October 20, 2026 08:51

Head branch was pushed to by a user without write access

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TCeason TCeason TCeason approved these changes

Assignees

No one assigned

Labels

pr-refactor this PR changes the code base without new features or bugfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Performance: time_slice rebuilds its constant slice spec for every row

2 participants

@vbmithr @TCeason