Skip to content

Restore configurable insert-method selection - #152

Open
Cianidos wants to merge 3 commits into
fix/issue-138-env-bridgesfrom
feat/issue-133-insert-method
Open

Restore configurable insert-method selection#152
Cianidos wants to merge 3 commits into
fix/issue-138-env-bridgesfrom
feat/issue-133-insert-method

Conversation

@Cianidos

Copy link
Copy Markdown
Contributor

Summary

Restores the configurable insert-method selection that buildDriverConfig had been silently discarding. defaultInsertMethod was parsed from presets, raw JSON, config files, and -D, but every workload kept its hard-coded InsertRequest.Method and invalid values succeeded.

What changed

  • New typed run parameter --insert-method (plain_query, plain_bulk, columnar, native) with process env INSERT_METHOD, -e INSERT_METHOD, and config run.insertMethod — the highest-precedence user override.
  • Driver-level defaultInsertMethod restored: preset/JSON/drivers[N].defaultInsertMethod/-D insertMethod/-D defaultInsertMethod now flows into config.DriverConfig.InsertMethod and is validated at driver-build time via driver.ParseInsertMethod.
  • driver.ResolveInsertMethod validates the value and rejects methods the selected driver does not serve — before loading starts.
  • Bench.Insert applies the resolved method to every load request (tpcc/tpcb/simple/tpch/tpcds all route through it); a zero value keeps the workload's own method.
  • Probe stays authoritative (driver.InsertCapabilities) and help/schema enumerate all values including columnar.

Precedence (highest → lowest)

  1. --insert-method (CLI > INSERT_METHOD env > -e > config run.insertMethod)
  2. driver-level defaultInsertMethod (-D/JSON > config drivers[N] > preset)
  3. workload-authored default

Tests

  • driver.ResolveInsertMethod/SupportsInsertMethod/InsertMethods unit tests (valid, invalid, unsupported).
  • bench resolution precedence test + a fake driver proving the resolved method reaches Driver.Insert (and that the workload default is kept when unset).
  • run.buildDriverConfig resolution tests (valid/invalid/unsupported + -D insertMethod aliases).

Closes #133

defaultInsertMethod was parsed from presets, JSON, config files, and -D but
discarded by buildDriverConfig, so every workload kept its hard-coded
InsertRequest.Method and invalid values passed silently.

Resolve an effective insert method once per run and apply it to every load:

- New run-scope typed parameter --insert-method (plus INSERT_METHOD env,
  -e, and config run.insertMethod) is the highest-precedence override.
- The driver-level defaultInsertMethod (preset, raw JSON -d, config
  drivers[N], -D insertMethod/-D defaultInsertMethod) is restored through
  buildDriverConfig and validated with driver.ParseInsertMethod.
- Unsupported-for-driver values fail before loading starts via
  driver.ResolveInsertMethod over the capability matrix.
- The workload-authored method remains the fallback when no override is set.

Precedence: typed CLI > env > config > driver default > workload default.

Closes #133
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d787fd23-76ea-4151-b597-a964bf732783

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

A bare -d preset was leaking its defaultInsertMethod into the effective
method, silently replacing the workload's hard-coded method. Presets are now
driver+URL templates and carry no insert-method opinion; only --insert-method,
-d/-D insertMethod/defaultInsertMethod, raw JSON, and config drivers[N] may
override. Also require a resolved driver type before validating the method and
capability-check the driver-set value through ResolveInsertMethod.

Closes #133
@Cianidos

Copy link
Copy Markdown
Contributor Author

Retarget blocked: runtime config design decision required

#152 cannot be mechanically detached from skipped #150/#151 and retargeted to current main.

A dry rebase of only this PR's commits (e4ad1b50..4d9af7e3) onto current main stopped on the first commit with conflicts in AGENTS.md, CHANGELOG.md, cmd/stroppy/commands/run/run.go, run_test.go, plus a modify/delete conflict for #150-only pkg/config/config.go.

Current main has a protobuf DriverRunConfig.defaultInsertMethod, but runtime DriverConfig has no field carrying the resolved method into bench.Run and drivers. This PR assumes #150's plain-Go runtime config and #151's removed environment bridges.

A maintainer must choose among functionally different designs:

  1. Extend the frozen protobuf runtime DriverConfig (field number, descriptor/reflection, JSON surface, getter/API).
  2. Keep protobuf unchanged and pass the resolved method separately through bench.Run or a new options type.
  3. Model it as another parameter-input source, changing source attribution and precedence behavior.

Two further policy choices affect observable behavior:

  • Keep preset defaultInsertMethod values, or remove them so bare presets cannot override workload-authored defaults.
  • Reject invalid lower-tier driver values even when a higher-precedence typed input wins, or validate only the selected effective value.

Merging this PR as-is updates #151's blocked branch, not main. Retargeting the existing head directly to main includes all seven blocked #150/#151 commits; the unique-only transplant requires the design choices above.

Please choose the runtime transport, preset-default policy, and validation timing, then restack only the selected #152 behavior on current main. Per the sequential merge rule, this PR is skipped; no branch changes were pushed.

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.

1 participant