Repository navigation
Let a Session pass arguments the engine needs at connect - #93
Merged
Merged
Conversation
#92 gave the addon a settings array and stopped there, because that change was about durable and this one is not. It is the half a caller can reach: new Session('./data', { connectionArgs: ['--async_load_databases=0', '--restore_threads=1', '--config-file=/etc/my-chdb.xml'], }) None of these can be a `SET`. `--config-file` is not a setting at all, and the arguments deciding how a data directory loads — `async_load_databases`, `async_load_system_database`, `tables_loader_foreground_pool_size`, `restore_threads` — have already taken effect by the time a query could run, so there is no later moment at which setting them would mean anything. A downstream moving off its own FFI bindings depends on all four, plus `output_format_json_quote_64bit_integers`, which is what keeps a value past 2^53 intact through JSON. Each is asserted against what the engine actually ended up with, read back out of `system.server_settings` and `system.settings`, rather than against the argv that went in: the addon can only prove which strings it passed, and the claim is about what the engine did with them. The config-file case writes a real XML and checks the value came from it. Two forms are refused, both already by the addon, so both surface here: `--path`, since the data directory is the connection registry's key and comes from the first argument, and anything containing a NUL byte, which truncates the argument at the C boundary and lets the next entry become its value. Nothing else is validated — an unknown setting name is the engine's to reject, not this layer's to guess at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@wudidapaopao please review this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-on to #92. That change gave the addon an optional settings array and
stopped there, because it was about durable and this is not — it is the half a
caller can actually reach.
Why these cannot be
SETs--config-fileis not a setting at all. And the arguments deciding how a datadirectory loads —
async_load_databases,async_load_system_database,tables_loader_foreground_pool_size,restore_threads— have already takeneffect by the time a query could run, so there is no later moment at which
setting them would mean anything. A downstream moving off its own FFI bindings
depends on all four, plus
output_format_json_quote_64bit_integers, which iswhat keeps a value past 2^53 intact through JSON.
Checked against engine
26.7.2-rc.2: all six take effect through connectarguments,
changed = 1on each, and a config file's values are read.The tests assert what the engine ended up with
Not the argv that went in. The addon can only prove which strings it passed,
and the claim is about what the engine did with them — so each case reads back
out of
system.server_settingsorsystem.settings:The config-file case writes a real XML and checks the value came from it. The
64-bit case asserts the round trip,
{"big":"9007199254740993"}, since past2^53 a JSON number would not survive it.
Two forms are refused
Both already by the addon, from #92, so both surface here:
--path, in any spelling. The data directory is the connectionregistry's key and comes from the first constructor argument; a setting that
moved it would leave the registry and the engine describing different places,
and the registry is what enforces one bound directory per process.
boundary, and a truncated prefix can be an option that takes the next entry
as its value — which is how
--pathgets smuggled past the check above. Thatwas measured on Bind the durable ABI in the addon, and ship the default Node engine #92:
['--path\0x', '/other']bound/otherwhile theregistry recorded the requested path.
Nothing else is validated. An unknown setting name or a malformed value is the
engine's to reject, not this layer's to guess at.
Tests
test/v3/connection-args.test.ts(new)npm run test:v3npm run test:durablenpm run test:durable:nodenpm run typecheckreports the same 13 pre-existing errors undertest/v3/**as on main.
npm test(mocha) cannot run on Node 26 locally —yargsfails toload — which is unchanged and unrelated.
The option is additive: absent, the addon is called with one argument, exactly
as every existing caller calls it, and a test pins that.
🤖 Generated with Claude Code
Note
Add
connectionArgsoption toSessionfor engine startup argumentsconnectionArgsstring array toSessionOptionsso engine command-line settings can be supplied at connect time, before the first query.Sessionconstructor validates that the option is an array of strings (throwingTypeErrorotherwise) and forwards it to native connection creation as a second argument; when absent, the existing one-argument call is unchanged.connectionArgsnow throw synchronously during construction in index.js; callers passing malformed values will see aTypeErrorrather than a silent ignore.📊 Macroscope summarized 42bc08f. 2 files reviewed, 1 issue evaluated, 1 issue filtered, 0 comments posted
🗂️ Filtered Issues
index.js — 0 comments posted, 1 evaluated, 1 filtered
connectionArgs.some(...)skips holes in a sparse array, sonew Session(path, { connectionArgs: ['--flag', , '--other'] })passes this validation even though the hole is not a string. The native wrapper subsequently reads that index asundefinedand throws, but the surroundingtryconverts itsTypeErrorintoChdbConnectionError; this violates the new API's promised synchronousTypeErrorvalidation for non-string entries and gives callers the wrong error category. [ Out of scope (post-validation triage) ]