Skip to content

Commit ca5d7b2

Browse files
bill-phclaude
andauthored
perf: publish properties with the nightly dataset and measure cached Trino JSON (#1224)
* perf: publish properties with the nightly dataset and measure cached Trino JSON One nightly frozen-perf run produced two unrelated datasets: the table suite published as posthog-file-views-v1 and the properties suite as properties-sha256-<inventory hash>, so the dashboard's dataset selector showed one or the other and no single view compared the engines across both. Publish both suites under the nightly's dataset version and tell them apart with a suite column (tables | properties) on runs and query_results. The properties inventory hash moves to a fixture_version column so history still never mixes fixtures. The publisher's schema bootstrap adds both columns and classifies existing rows by the -properties run ID suffix; a summary without a suite publishes as tables, and an unknown suite is rejected. Also measure the properties JSON queries on trino_cached ("trino (cache)"). It only ran the unsupported VARIANT representation, so cached Trino had no properties coverage; now that the Hoglake connector honors fs.cache.enabled, the measurement is meaningful. Update the README paragraph that still said the connector ignores the cache. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * perf: pair suites by nightly_run_id and migrate the schema ahead of data Review fixes: - Publish an explicit nightly_run_id (the table-suite run's ID) instead of leaving consumers to strip a -properties suffix from run IDs. - Make the column migration one-shot: a guarded DO block adds suite, fixture_version and nightly_run_id and classifies older rows only when suite is missing, instead of two UPDATE scans on every publish. - Add duckgres-perf-publisher --bootstrap-only and run it at the start of main scenario runs, so the schema (and a dashboard that depends on it) can land hours before the first data in the new shape; no window where the old dashboard picks the properties run as the latest run. - Move the suite constants to core and validate the suite when the step starts, so a typo fails before the benchmark instead of at publish. - Share the nightly dataset version through a YAML anchor. - README: correct the uncached-cache cutoff and stop claiming that fixture_version alone keeps history from spanning fixture changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * perf: drop the bootstrap-only mode and the one-shot migration Both solved problems that did not justify permanent machinery. The bootstrap-only CLI mode and workflow step existed to close a one-time window of a few hours where the dev perf dashboard's table panels would be empty; merging the dashboard change right after the first publish does the same. The guarded DO-block migration avoided two UPDATE scans on a table that grows by ~200 rows a night; plain idempotent ALTERs and a WHERE suite IS NULL backfill are simpler and just as correct. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 197f3b6 commit ca5d7b2

14 files changed

Lines changed: 364 additions & 49 deletions

File tree

‎docs/runbooks/properties-perf.md‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ fixture catalogs and metadata without deleting source S3 files.
6868
| duckgres (vanilla) | Off | JSON |
6969
| duckgres (cache) | On | JSON |
7070
| trino (vanilla) | Off | JSON |
71+
| trino (cache) | On | JSON |
7172
| trino (cache+variant) | On | VARIANT — explicitly skipped |
7273
| Athena | — | STRUCT |
7374

@@ -83,15 +84,15 @@ do not substitute JSON under a VARIANT label.
8384

8485
## Results and recovery
8586

86-
Original results retain `perf/`, the original run ID, and dataset version.
87-
Properties use `perf-properties/`, a `-properties` run ID suffix, and an
88-
inventory-derived dataset version. Both summaries are handled by the existing
89-
publisher; only main-branch runs publish to the historical database. A
87+
Both suites publish under the nightly's dataset version with a `suite` column
88+
(`tables` / `properties`). Properties use `perf-properties/`, a `-properties`
89+
run ID suffix, and record the inventory-derived hash as `fixture_version`. Both
90+
summaries are handled by the existing publisher; only main-branch runs publish
91+
to the historical database. A
9092
properties failure still fails the scenario, but previously completed original
9193
results remain available for upload and publication. No timing result is
9294
claimed for a skipped comparison. Dashboard comparisons should filter
93-
`status = 'ok'` and use the matching properties intent; original aggregate
94-
panels retain their existing query selection.
95+
`status = 'ok'` and select the suite with the `suite` column.
9596

9697
For missing/inaccessible prefixes, correct the selection or access and rerun.
9798
For Athena mapping failures, use the generated SQL and approved fixture

‎tests/mw-dev/scenario/perf/adapter_test.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,10 @@ func TestExecutorRunsPerfStepAndWritesArtifacts(t *testing.T) {
6666
if result.Summary.RunID != "scenario-run-1" || result.Summary.TotalQueries != 1 || result.Summary.TotalErrors != 0 {
6767
t.Fatalf("summary = %+v", result.Summary)
6868
}
69+
// A perf step with no suite is the table suite and its own nightly.
70+
if result.Summary.Suite != perfcore.SuiteTables || result.Summary.NightlyRunID != "scenario-run-1" {
71+
t.Fatalf("summary suite/nightly = %q/%q", result.Summary.Suite, result.Summary.NightlyRunID)
72+
}
6973
pgwireDSN := factory.pgwireConnection.DSN
7074
if !strings.Contains(pgwireDSN, "host=scenario-org.dev.example") || !strings.Contains(pgwireDSN, "password=root-password") {
7175
t.Fatalf("pgwire dsn = %q, want scenario org host and provision password", pgwireDSN)
@@ -99,6 +103,36 @@ func TestExecutorRunsPerfStepAndWritesArtifacts(t *testing.T) {
99103
}
100104
}
101105

106+
func TestExecutorStampsSuiteMetadataAndRejectsUnknownSuites(t *testing.T) {
107+
targets := []perfcore.Protocol{perfcore.ProtocolPGWire}
108+
run := func(extra map[string]any) (*Executor, error) {
109+
executor := NewExecutor(ExecutorConfig{
110+
Connection: scenariosql.ConnectionConfig{DialHost: "127.0.0.1", SNISuffix: ".example.test", SSLMode: "require"},
111+
OutputDir: t.TempDir(), DriverFactory: &fakeDriverFactory{},
112+
})
113+
with := map[string]any{
114+
"org_id": "scenario-org", "username": "root", "password": "test-password",
115+
"catalog_file": writePerfCatalog(t, targets), "run_id": "nightly-1-properties",
116+
}
117+
for key, value := range extra {
118+
with[key] = value
119+
}
120+
return executor, executor.ExecuteStep(context.Background(), core.Step{ID: "properties", Type: StepTypePerfQueries, With: with})
121+
}
122+
executor, err := run(map[string]any{"suite": "properties", "fixture_version": "properties-sha256-abc", "nightly_run_id": "nightly-1"})
123+
if err != nil {
124+
t.Fatal(err)
125+
}
126+
result, _ := executor.State().Result("properties")
127+
if result.Summary.Suite != perfcore.SuiteProperties || result.Summary.FixtureVersion != "properties-sha256-abc" || result.Summary.NightlyRunID != "nightly-1" {
128+
t.Fatalf("summary = %+v", result.Summary)
129+
}
130+
// A typo fails before any measurement, not hours later at publish time.
131+
if _, err := run(map[string]any{"suite": "property"}); err == nil || !strings.Contains(err.Error(), "unknown suite") {
132+
t.Fatalf("expected unknown suite error, got %v", err)
133+
}
134+
}
135+
102136
func TestExecutorRecordsBothPGWireCacheVariantsWithEqualResources(t *testing.T) {
103137
targets := []perfcore.Protocol{perfcore.ProtocolPGWireUncached, perfcore.ProtocolPGWireCached}
104138
factory := &fakeDriverFactory{}

‎tests/mw-dev/scenario/perf/steps.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,9 @@ type stepSpec struct {
7171
Targets []perfcore.Protocol
7272
RunID string
7373
DatasetVersion string
74+
Suite string
75+
FixtureVersion string
76+
NightlyRunID string
7477
Database string
7578
OutputSubdir string
7679
ReadOnly bool
@@ -203,6 +206,9 @@ func (e *Executor) ExecuteStep(ctx context.Context, step core.Step) error {
203206
RunID: spec.RunID,
204207
Catalog: catalog,
205208
DatasetVersion: spec.DatasetVersion,
209+
Suite: spec.Suite,
210+
FixtureVersion: spec.FixtureVersion,
211+
NightlyRunID: spec.NightlyRunID,
206212
Drivers: drivers,
207213
Sink: closingSink{sink: sink, closeFunc: closeSink},
208214
Now: e.now,
@@ -250,6 +256,10 @@ func (e *Executor) parseStep(step core.Step) (stepSpec, error) {
250256
if err != nil {
251257
return stepSpec{}, err
252258
}
259+
suite := stringFromWith(step, "suite", perfcore.SuiteTables)
260+
if err := perfcore.ValidateSuite(suite); err != nil {
261+
return stepSpec{}, classified(ErrorClassConfig, err)
262+
}
253263
if e.outputDir == "" {
254264
return stepSpec{}, classified(ErrorClassConfig, fmt.Errorf("perf output dir is required"))
255265
}
@@ -298,6 +308,9 @@ func (e *Executor) parseStep(step core.Step) (stepSpec, error) {
298308
Targets: targets,
299309
RunID: runID,
300310
DatasetVersion: stringFromWith(step, "dataset_version", ""),
311+
Suite: suite,
312+
FixtureVersion: stringFromWith(step, "fixture_version", ""),
313+
NightlyRunID: stringFromWith(step, "nightly_run_id", runID),
301314
Database: stringFromWith(step, "catalog", "ducklake"),
302315
OutputSubdir: stringFromWith(step, "output_subdir", "perf"),
303316
ReadOnly: boolFromWith(step, "read_only", true),

‎tests/mw-dev/scenario/runner_properties_test.go‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/posthog/duckgres/tests/mw-dev/scenario/core"
1515
scenarioperf "github.com/posthog/duckgres/tests/mw-dev/scenario/perf"
1616
scenariosql "github.com/posthog/duckgres/tests/mw-dev/scenario/sql"
17+
perfcore "github.com/posthog/duckgres/tests/perf/core"
1718
"github.com/posthog/duckgres/tests/perf/properties"
1819
"gopkg.in/yaml.v3"
1920
)
@@ -23,6 +24,12 @@ const stepTypePropertiesComparison = "properties_comparison"
2324
// This phase runs only after the original benchmarks have written their
2425
// artifacts. Fixture preparation errors cannot prevent those results publishing.
2526
func (e dispatchExecutor) runPropertiesComparison(ctx context.Context, step core.Step) error {
27+
// Properties results publish under the nightly's dataset so one dashboard
28+
// selection covers every suite; the fixture hash is kept as fixture_version.
29+
datasetVersion, _ := step.With["dataset_version"].(string)
30+
if datasetVersion == "" {
31+
return fmt.Errorf("properties comparison requires the nightly dataset_version")
32+
}
2633
source := os.Getenv("DUCKGRES_SCENARIO_PROPERTIES_S3_URI")
2734
prepared, err := preparePropertiesComparison(ctx, source, step)
2835
if err != nil {
@@ -43,13 +50,18 @@ func (e dispatchExecutor) runPropertiesComparison(ctx context.Context, step core
4350
}}); err != nil {
4451
return err
4552
}
46-
with := make(map[string]any, len(step.With)+3)
53+
with := make(map[string]any, len(step.With)+6)
4754
for key, value := range step.With {
4855
with[key] = value
4956
}
5057
with["catalog_file"] = filepath.Join(prepared.directory, "catalog.yaml")
51-
with["dataset_version"] = "properties-sha256-" + prepared.dataset.SHA256
58+
with["dataset_version"] = datasetVersion
59+
with["suite"] = perfcore.SuiteProperties
60+
with["fixture_version"] = "properties-sha256-" + prepared.dataset.SHA256
5261
with["output_subdir"] = "perf-properties"
62+
// Distinct run ID so the publisher never overwrites the table-suite run;
63+
// consumers pair the two through nightly_run_id, not this suffix.
64+
with["nightly_run_id"] = fmt.Sprint(step.With["run_id"])
5365
with["run_id"] = fmt.Sprint(step.With["run_id"]) + "-properties"
5466
return e.perf.ExecuteStep(ctx, core.Step{ID: step.ID, Type: scenarioperf.StepTypePerfQueries, With: with})
5567
}

‎tests/mw-dev/scenario/runner_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -554,6 +554,11 @@ func TestFrozenPerfScenarioRunsOptionalPropertiesAfterOriginalPerf(t *testing.T)
554554
if got := comparison.DependsOn; len(got) != 1 || got[0] != "perf_queries" {
555555
t.Fatalf("properties dependencies = %#v, want original perf first", got)
556556
}
557+
// One nightly publishes both suites under one dataset, so the dashboard's
558+
// comparison sees them together.
559+
if got, want := comparison.With["dataset_version"], steps["perf_queries"].With["dataset_version"]; got == nil || got != want {
560+
t.Fatalf("properties dataset_version = %#v, want the perf_queries dataset %#v", got, want)
561+
}
557562
for _, name := range scenario.RequiredEnv {
558563
if name == "DUCKGRES_SCENARIO_PROPERTIES_S3_URI" {
559564
t.Fatal("properties fixture must remain optional")

‎tests/mw-dev/scenario/scenarios/posthog_frozen_perf.yaml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ steps:
111111
athena_poll_interval: 500ms
112112
athena_query_timeout: 30m
113113
run_id: ${run_id}
114-
dataset_version: posthog-file-views-v1
114+
dataset_version: &nightly_dataset posthog-file-views-v1
115115
fail_on_query_errors: true
116116
worker_cpu: ${env:DUCKGRES_K8S_WORKER_CPU_REQUEST}
117117
worker_memory: ${env:DUCKGRES_K8S_WORKER_MEMORY_REQUEST}
@@ -139,6 +139,8 @@ steps:
139139
athena_poll_interval: 500ms
140140
athena_query_timeout: 30m
141141
run_id: ${run_id}
142+
# Same dataset as perf_queries: both suites of one nightly publish together.
143+
dataset_version: *nightly_dataset
142144
output_subdir: perf-properties
143145
fail_on_query_errors: true
144146
worker_cpu: ${env:DUCKGRES_K8S_WORKER_CPU_REQUEST}

‎tests/perf/README.md‎

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -260,11 +260,11 @@ queries/iterations until eviction or teardown; one warm-up does not guarantee
260260
every replica holds the complete working set. Worker CPU/memory limits remain
261261
three workers at 1 CPU/4Gi per cluster; cache volumes reserve additional storage.
262262

263-
The current frozen suite uses the pinned Hoglake connector, which ignores
264-
`fs.cache.enabled`; actual filesystem-cache support remains deferred. Thus
265-
`trino_cached` currently labels the requested configuration, not verified cache
266-
hits or a demonstrated cache benefit. Consolidating the scenario does not change
267-
that connector behavior. The baseline also does not guarantee cold JVM, OS, or
263+
The frozen suite runs the newest PostHog/trino master build, whose Hoglake
264+
connector honors `fs.cache.enabled` through Trino's shared filesystem module
265+
(PostHog/trino#43). Builds before that change ignored the property, so every
266+
`trino_cached` result from the pinned build, through the 2026-09-23 08:35 UTC
267+
nightly, measured an uncached cluster under the cached label. The baseline also does not guarantee cold JVM, OS, or
268268
storage-service caches. Keep the protocol labels separate in history.
269269

270270
Trino `physicalInputBytes` can include bytes served from cache. Use cache-manager
@@ -287,22 +287,33 @@ iterations. Merely filtering a large mixed-day file set does not guarantee small
287287
scans. The runner does not generate data or enforce a row-count limit.
288288

289289
The properties catalog measures JSON with `duckgres (vanilla)`, `duckgres (cache)`,
290-
and `trino (vanilla)`, plus STRUCT with Athena. Athena executes only STRUCT;
290+
`trino (vanilla)` and `trino (cache)`, plus STRUCT with Athena, so every engine
291+
covers every properties intent. Athena executes only STRUCT;
291292
its complete ordered results must match the shared Duckgres/Trino JSON baseline
292293
for each intent before properties measurements start. `trino (cache+variant)` is explicitly
293294
unsupported until Hoglake supports VARIANT: its two comparisons emit `skipped`
294-
rows with a reason and no timings, and do not connect to cached Trino. Skipped
295-
rows use iteration zero and are excluded from measured/warmup query counts.
296-
Original cached Trino benchmarks continue running normally.
297-
298-
Original results retain `perf/`, the scenario run ID, and
299-
`posthog-file-views-v1`. Properties results use `perf-properties/`, a
300-
`-properties` run ID suffix, and a version derived from the selected object
301-
inventory. This keeps the original dashboard history stable and prevents the
302-
publisher from overwriting one result set with the other. Both use the existing
303-
publisher; only main-branch runs publish to the shared database. Dashboard
304-
comparisons must match properties intent IDs and successful statuses; existing
305-
full-corpus aggregate panels do not automatically include properties queries.
295+
rows with a reason and no timings. Skipped rows use iteration zero and are
296+
excluded from measured/warmup query counts.
297+
298+
Both suites of one nightly publish under the same dataset version
299+
(`posthog-file-views-v1`, shared through a YAML anchor in the scenario), with
300+
three columns on `runs` and `query_results`:
301+
302+
- `suite` (`tables` | `properties`, a closed set in `core`, validated when the
303+
step starts so a typo fails before hours of measurement);
304+
- `nightly_run_id`, the table-suite run's ID, which pairs a nightly's suites
305+
explicitly (a standalone run is its own nightly);
306+
- `fixture_version`, the hash of the selected properties object inventory.
307+
308+
The properties suite keeps its own `perf-properties/` directory and a distinct
309+
`-properties` run ID, so the publisher never overwrites one result set with the
310+
other. `fixture_version` records which fixture each properties run measured; it
311+
does not by itself stop a history chart from spanning a fixture change, so
312+
regenerating the fixture is a deliberate history break. The publisher's schema
313+
bootstrap adds the columns and classifies rows published before they existed
314+
(by the `-properties` suffix they used to carry); rows with explicit values are
315+
never touched. A summary without a suite publishes as `tables`. Only
316+
main-branch runs publish to the shared database.
306317

307318
All properties preparation happens after the original result files are complete,
308319
so a properties setup or validation failure cannot prevent their publication.

‎tests/perf/core/runner.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ type RunnerConfig struct {
2424
RunID string
2525
Catalog Catalog
2626
DatasetVersion string
27+
Suite string
28+
FixtureVersion string
29+
NightlyRunID string
2730
Drivers map[Protocol]ProtocolDriver
2831
Sink ResultSink
2932
OnSetup func(context.Context) error
@@ -57,6 +60,9 @@ func (r *QueryRunner) Run(ctx context.Context) (RunSummary, error) {
5760
summary := RunSummary{
5861
RunID: runID,
5962
DatasetVersion: r.cfg.DatasetVersion,
63+
Suite: r.cfg.Suite,
64+
FixtureVersion: r.cfg.FixtureVersion,
65+
NightlyRunID: r.cfg.NightlyRunID,
6066
StartedAt: startedAt,
6167
FinishedAt: startedAt,
6268
}

‎tests/perf/core/runner_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,29 @@ func summaryProtocolIterations(sink *inMemorySink) []string {
8282
return got
8383
}
8484

85+
func TestRunnerStampsSuiteAndFixtureVersionOnTheSummary(t *testing.T) {
86+
runner := NewQueryRunner(RunnerConfig{
87+
RunID: "run-properties",
88+
DatasetVersion: "posthog-file-views-v1",
89+
Suite: "properties",
90+
FixtureVersion: "properties-sha256-abc",
91+
Catalog: Catalog{
92+
Name: "suite", MeasureIterations: 1, Targets: []Protocol{ProtocolPGWire},
93+
Queries: []Query{{QueryID: "q1", IntentID: "i1", PGWireSQL: "SELECT 1"}},
94+
},
95+
Drivers: map[Protocol]ProtocolDriver{ProtocolPGWire: &testDriver{protocol: ProtocolPGWire}},
96+
Sink: &inMemorySink{},
97+
Now: func() time.Time { return time.Unix(1700000000, 0) },
98+
})
99+
summary, err := runner.Run(context.Background())
100+
if err != nil {
101+
t.Fatalf("Run returned error: %v", err)
102+
}
103+
if summary.DatasetVersion != "posthog-file-views-v1" || summary.Suite != "properties" || summary.FixtureVersion != "properties-sha256-abc" {
104+
t.Fatalf("summary = %+v", summary)
105+
}
106+
}
107+
85108
func TestRunnerExecutesPairedQueriesThroughExistingRuntimeContract(t *testing.T) {
86109
catalog, err := ParseCatalog([]byte(pairedCatalogYAML(`
87110
paired_queries:

‎tests/perf/core/types.go‎

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package core
22

33
import (
4+
"fmt"
45
"strings"
56
"time"
67
)
@@ -26,6 +27,8 @@ func (p Protocol) RunLabel(representation string) string {
2627
return "duckgres (cache)"
2728
case p == ProtocolTrino && representation == "json":
2829
return "trino (vanilla)"
30+
case p == ProtocolTrinoCached && representation == "json":
31+
return "trino (cache)"
2932
case p == ProtocolTrinoCached && representation == "variant":
3033
return "trino (cache+variant)"
3134
default:
@@ -113,14 +116,38 @@ type QueryResult struct {
113116
ServiceMetrics *ServiceMetrics `json:"service_metrics,omitempty"`
114117
}
115118

119+
// Suites published under one dataset. Dashboards filter on this closed set.
120+
const (
121+
SuiteTables = "tables"
122+
SuiteProperties = "properties"
123+
)
124+
125+
// ValidateSuite rejects a suite outside the closed set, so a typo fails when a
126+
// run starts rather than after hours of measurement at publish time.
127+
func ValidateSuite(suite string) error {
128+
if suite != SuiteTables && suite != SuiteProperties {
129+
return fmt.Errorf("unknown suite %q (want %q or %q)", suite, SuiteTables, SuiteProperties)
130+
}
131+
return nil
132+
}
133+
116134
type RunSummary struct {
117-
RunID string `json:"run_id"`
118-
DatasetVersion string `json:"dataset_version"`
119-
StartedAt time.Time `json:"started_at"`
120-
FinishedAt time.Time `json:"finished_at"`
121-
TotalQueries int `json:"total_queries"`
122-
TotalErrors int `json:"total_errors"`
123-
WarmupQueries int `json:"warmup_queries"`
135+
RunID string `json:"run_id"`
136+
DatasetVersion string `json:"dataset_version"`
137+
// Suite names the comparison family inside a dataset ("tables" or
138+
// "properties"); one nightly publishes several suites under one dataset.
139+
Suite string `json:"suite,omitempty"`
140+
// FixtureVersion identifies the exact fixture a suite measured when it is
141+
// finer-grained than the dataset (e.g. the properties object inventory).
142+
FixtureVersion string `json:"fixture_version,omitempty"`
143+
// NightlyRunID names the run every suite of one nightly belongs to: the
144+
// table-suite run's ID. Consumers pair suites by it rather than by run IDs.
145+
NightlyRunID string `json:"nightly_run_id,omitempty"`
146+
StartedAt time.Time `json:"started_at"`
147+
FinishedAt time.Time `json:"finished_at"`
148+
TotalQueries int `json:"total_queries"`
149+
TotalErrors int `json:"total_errors"`
150+
WarmupQueries int `json:"warmup_queries"`
124151
}
125152

126153
// SQLFor preserves canonical SQL except for the JSON scalar extractor in the

0 commit comments

Comments
 (0)