Skip to content

sp_HumanEvents: allow sorting query results by event_time#795

Open
kendra-little wants to merge 3 commits into
erikdarlingdata:devfrom
kendra-little:order-byeeeeee
Open

sp_HumanEvents: allow sorting query results by event_time#795
kendra-little wants to merge 3 commits into
erikdarlingdata:devfrom
kendra-little:order-byeeeeee

Conversation

@kendra-little
Copy link
Copy Markdown
Contributor

@kendra-little kendra-little commented May 22, 2026

Summary

  • Adds "event_time" as a valid value for @query_sort_order in sp_HumanEvents, so query results can be ordered newest-first by capture time.
  • event_time is converted to bigint via DATEDIFF_BIG(MILLISECOND, '20000101', q.event_time) inside the existing ORDER BY CASE, keeping the CASE branches uniformly numeric (avoids "numeric is incompatible with datetime2").
  • README accepted-values cell updated to list "event_time".

Use Case

I want to observe what commands were run in their given order in a development environment.

🤖 Generated with Claude Code

erikdarlingdata and others added 3 commits May 21, 2026 09:58
Merge dev to main: sp_QuickieStore high-impact plan_count fix
Adds "event_time" as a valid value for @query_sort_order so query
results can be ordered newest-first by capture time. event_time is
converted to a bigint via DATEDIFF_BIG so it shares the numeric
result type of the existing CASE branches.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Owner

@erikdarlingdata erikdarlingdata left a comment

Choose a reason for hiding this comment

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

Reviewed the event_time sort change — the core T-SQL is sound. Verified:

  • q.event_time is a real column in the query_results CTE / final SELECT scope, so it's valid in the ORDER BY.
  • DATEDIFF_BIGbigint keeps every CASE branch numeric, avoiding the datetime2/numeric type clash — correct reasoning.
  • DATEDIFF_BIG is available in SQL Server 2016+ (MS Learn), which matches the project's minimum supported version — no compatibility concern.
  • Validation list, @help text, and README are all updated consistently.
  • The ELSE N'cpu'ELSE q.total_cpu_ms change is a genuine latent bug fix (a string literal in an otherwise-numeric CASE would fail conversion if ever reached). Good catch — worth keeping.

A few things to address:

1. Install-All/DarlingData.sql should be reverted out of this PR.
That file is auto-regenerated by the build-sqlfile.yml workflow on push to main. The committed change here is both out of scope and inconsistent: it contains the unrelated sp_QuickieStore fix from #793 (picked up by a local Merge-All.ps1 run) but does not contain this PR's event_time change — so the combined file is now out of sync with its own source. Cleanest fix: revert Install-All/DarlingData.sql to base and let CI rebuild it on merge to main.

2. Semantic nuance — the sorted event_time is an arbitrary representative, not strictly "newest".
The final result set is deduped by ROW_NUMBER() partitioned by the query/plan hashes, with the ORDER BY inside that window using the same hash columns. So n = 1 picks an arbitrary execution per query group, and its event_time is an arbitrary timestamp among that query's runs — not necessarily the latest. For the stated use case (a dev environment where statements typically run once) this is fine in practice, but it doesn't strictly deliver "ordered newest-first by capture time." If truly-latest matters, the CTE's ROW_NUMBER ORDER BY would need q.event_time DESC (which also changes which statement_text/plan is shown — a bigger change). Flagging so it's a conscious decision.

3. Minor style nits:

  • DATEDIFF_BIG(MILLISECOND, '20000101', q.event_time) is inline; the codebase formats multi-arg functions multi-line (see the DATEDIFF(...) block a few lines below in the compile-events branch). Defensible inside a CASE branch for brevity, but inconsistent with the style guide.
  • Help/README wording "...", "spills", "event_time", and you can add "avg"... could read as if avg event_time is valid — consider rephrasing to make clear event_time has no avg variant.

4. Pre-existing, not introduced here (FYI): @query_sort_order is declared nvarchar(10), but 'avg duration' is 12 chars and silently truncates today. event_time is exactly 10 chars so it just fits. The parameter is undersized — worth widening in a separate fix.

Net: approvable once the Install-All file is reverted; items 2–4 are comments rather than blockers.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants