[peco-engineer-bot]

Summary

Automated fix for #261 — Batch INSERT (using array of arrays) results in the application hanging.

On the Thrift path, getQueryParameters (lib/thrift-backend/ThriftSessionBackend.ts) wrapped every value in a DBSQLParameter without checking its shape. So arrays and plain objects (like ordinalParameters: [[1],[2],[3]]) went to the server as STRING params with a raw stringValue, and the operation never finished. Now every named and ordinal value goes through the existing assertBindableValue check (the one the kernel path already uses), so these values throw ParameterError straight away instead of hanging. Batch binding of row arrays isn't possible because Thrift's TSparkParameter holds only one scalar per placeholder, so failing fast is the "or failing" outcome the issue asks for. The new e2e tests pass against the live warehouse, the unit tests pass, npm test passes (1490), and lint and prettier are clean. npm run type-check fails only in examples/tokenFederation/*.ts, which can't find @databricks/sql; this change doesn't touch those files.

Root cause & plan

Root cause: On the Thrift backend, getQueryParameters (lib/thrift-backend/ThriftSessionBackend.ts) wraps every ordinal/named value in new DBSQLParameter({ value }) with no shape validation. A non-scalar value such as an array (ordinalParameters: [[1],[2],[3]]) or a plain object falls through every branch of DBSQLParameter.toSparkParameter to the STRING default, producing a TSparkParameter whose stringValue is the raw array/object. That garbage parameter is sent over the wire; the request/operation never reaches a terminal state, so the app hangs instead of failing. The kernel backend already guards against this via assertBindableValue (lib/kernel/KernelInputValidation.ts, whose own doc comment calls out the Thrift-path hang as a DoS hazard), but the Thrift path never calls it. Batch binding of row arrays isn't supported by the TSparkParameter protocol (it carries one scalar per placeholder), so the right behavior is to fail fast with a ParameterError, as the issue's "working (or failing)" expectation asks.
Files: lib/thrift-backend/ThriftSessionBackend.ts, tests/e2e/query_parameters.test.ts, tests/unit/DBSQLSession.test.ts
Planned coverage:

• tests/e2e/query_parameters.test.ts: open a session against the live warehouse and run session.executeStatement('SELECT ? AS c', { ordinalParameters: [[1],[2],[3]] as never }), then fetchAll. Assert it rejects with ParameterError (message mentions array) quickly instead of hanging. Use a bounded mocha timeout / Promise.race so a hang shows up as a test failure, not a stuck suite. Today it hangs or returns no ParameterError -> red. (ordinal array-of-arrays (issue repro) on Thrift)
• Same e2e file: executeStatement('SELECT :p AS c', { namedParameters: { p: [1, 2] as never } }) must reject with ParameterError rather than hang or send a garbage STRING param. (named parameter with array value on Thrift)
• Same e2e file: ordinalParameters: [{ a: 1 } as never] must reject with ParameterError (non-scalar object), matching kernel's assertBindableValue behavior. (plain-object parameter value on Thrift)
• Optional extra unit test in tests/unit/DBSQLSession.test.ts: executeStatement with array/object ordinal and named values throws ParameterError and the driver's executeStatement stub is never called. Valid scalar values (Date, Int64, bigint, null) keep working. (unit: getQueryParameters validation offline)

Files changed

• tests/e2e/query_parameters.test.ts
• tests/unit/DBSQLSession.test.ts
• lib/thrift-backend/ThriftSessionBackend.ts

Test plan

• tests/e2e/query_parameters.test.ts: Query parameters non-scalar parameter values (issue #261) should reject an array-of-arrays of ordinal parameters with ParameterError instead of hanging — fails (red) against the original code, passes (green) after the fix
• tests/e2e/query_parameters.test.ts: Query parameters non-scalar parameter values (issue #261) should reject an array value for a named parameter with ParameterError — fails (red) against the original code, passes (green) after the fix
• tests/e2e/query_parameters.test.ts: Query parameters non-scalar parameter values (issue #261) should reject a plain-object ordinal parameter value with ParameterError — fails (red) against the original code, passes (green) after the fix
• tests/unit/DBSQLSession.test.ts: DBSQLSession executeStatement with non-scalar parameter values should throw ParameterError and not call the driver for ordinal array-of-arrays — fails (red) against the original code, passes (green) after the fix
• tests/unit/DBSQLSession.test.ts: DBSQLSession executeStatement with non-scalar parameter values should throw ParameterError and not call the driver for named array value — fails (red) against the original code, passes (green) after the fix
• tests/unit/DBSQLSession.test.ts: DBSQLSession executeStatement with non-scalar parameter values should throw ParameterError and not call the driver for ordinal plain object — fails (red) against the original code, passes (green) after the fix
• tests/unit/DBSQLSession.test.ts: DBSQLSession executeStatement with non-scalar parameter values should throw ParameterError and not call the driver for named plain object — fails (red) against the original code, passes (green) after the fix

🤖 Generated by engineer-bot (bug-fix flow) — review before merge.

[peco-review-bot]

Verdict: 1 Medium · 1 Low · 1 Nit

Good fix that fails fast on bare array/object parameters on the Thrift path. One medium gap: a non-scalar wrapped in DBSQLParameter skips the new guard, so it can still send a raw array/object as stringValue. There are also two low notes, on where the shared validator lives and on test coverage.

Other findings

• ⚪ Nit — The unit tests leave out the case where the gap in F1 lives: a DBSQLParameter that wraps an array or object. The "valid scalar values" test also skips undefined, boolean, number, and a DBSQLParameter instance, which are all on the allow-list the guard now enforces. If you add those cases, regressions in either direction (over-rejecting or under-rejecting) would show up in the unit suite.

[peco-review-bot]

✅ No issues identified by the review bot.

[vuanhphung]

@bot run prettier to fix lint issue

[peco-engineer-bot]

Could not apply the requested change — I finished without editing any files. Please rephrase or add specifics.

Started from 5798e32. I ran Prettier and it reformatted one file, tests/unit/kernel/inputValidation.test.ts. That was the only file npm run prettier flagged, and the change is formatting only. npm run prettier now passes ("All matched files use Prettier code style!"). After npm run build, the 5 tests in that file still pass. I could not check npm run lint here: ESLint stopped because it couldn't find the airbnb-base config, which is referenced from the .eslintrc in the runner's parent directory, outside the PR checkout. So lint needs to be confirmed in CI. I did not run the full unit suite, type-check or e2e tests.

[vuanhphung]

@bot Fix the formatting in tests/unit/kernel/inputValidation.test.ts using edit_file so the engine records the changes. Do not use --write, --fix, or prettier:fix.

[peco-engineer-bot]

Pushed 68ddcd0 to ai/issue-261 addressing your request.

Started from 5798e32. Fixed the formatting in tests/unit/kernel/inputValidation.test.ts with edit_file. The first DBSQLParameter array expect in 'rejects non-scalar values wrapped in DBSQLParameter' is now laid out the way Prettier 2.8 wants: expect(() => …).to.throw( on one line, with the arguments split across lines. npm run prettier now passes. The 5 tests in that file pass, and npm test passes (1492 tests). npm run lint didn't run because the airbnb-base ESLint config isn't installed here. npm run type-check fails only in examples/tokenFederation, which can't find the @databricks/sql module. Neither failure involves this file. No e2e tests were run.

[peco-review-bot]

Verdict: 1 Low · 1 Nit

Looks good. The fix correctly routes Thrift-path named and ordinal values through the shared assertBindableValue gate, which now lives in a backend-neutral module with a compatibility re-export. Two low notes: the wrapped-DBSQLParameter check now applies to the kernel backend too, and the e2e cases are heavier than they need to be. One nit: the describe('KernelInputValidation.assertBindableValue') label in tests/unit/kernel/inputValidation.test.ts is now stale, since the function moved to lib/utils.

Other findings

• ⚪ Nit — The new check runs client-side, before driver.executeStatement is called. So these e2e cases never send anything to the warehouse once the fix is in. Each one still opens and closes a live session, and each gets a 60s mocha timeout plus a 30s hang-race scaffold. The unit tests in tests/unit/DBSQLSession.test.ts already prove the driver isn't called. You could keep a single e2e regression case for the issue repro and drop the other two to cut warehouse time. As written they're correct, just more expensive than needed.

[peco-review-bot]

Verdict: 1 Low

The fix looks correct, with one low edge-case gap. The Thrift path now runs the shared assertBindableValue gate, so arrays and objects are rejected with a ParameterError before the driver is called, and unit tests cover both backends. Nit: no code in the repo still imports the lib/kernel/KernelInputValidation.ts re-export (only a describe label mentions it), so it could be removed unless external deep imports need it.

[peco-review-bot]

Verdict: 2 Low

Looks good overall. Moving assertBindableValue into a backend-neutral spot and calling it in Thrift's getQueryParameters fixes the hang reported in #261, and the unit tests are solid. Two low-severity notes: the e2e hang harness is redundant, and a few input types that used to pass on Thrift are now rejected (worth a CHANGELOG line).

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity test gap. Both backends now use the same bind-time check, and the fix matches the root cause: non-scalar values used to fall through to the STRING default on the Thrift path. Nit: the e2e test should support multi-row inserts with flattened scalar ordinal parameters runs a SELECT ... FROM VALUES, not an INSERT, so a name like 'multi-row VALUES with flattened scalar ordinal parameters' would describe it more accurately.

[peco-review-bot]

Verdict: 1 Low

Looks good. The fix correctly sends Thrift named and ordinal values through the shared assertBindableValue gate before the driver RPC, and the unit tests confirm the driver is never called for rejected values. One low finding: lib/kernel/KernelInputValidation.ts is now an unused re-export. Nits: the e2e test titled 'multi-row inserts' actually runs a SELECT ... FROM VALUES, and the describe('KernelInputValidation.assertBindableValue') label should be renamed now that the function lives in lib/utils/.

[peco-review-bot]

🔵 Low — Nothing in lib/ imports this module any more. KernelPositionalParams.ts and the unit test both import ../utils/assertBindableValue directly. That leaves it as an internal re-export "for compatibility" with nothing that depends on it. If lib/kernel/* isn't a supported public import path, deleting the file is simpler and leaves one source of truth. If it is meant to stay, add a @deprecated JSDoc so callers know to move to the new path.

[peco-engineer-bot]

The type-check errors are only in examples/ (missing @databricks/sql module resolution), which existed before this change and aren't related to it.

Pushed cd42145.

[peco-review-bot]

Thanks for pushing cd42145. At head, lib/kernel/KernelInputValidation.ts still has the plain compatibility re-export, with no @deprecated JSDoc, and the file hasn't been removed. The commit may have changed other files, but it doesn't touch the point this thread raised. A human should pick one of two options: delete the file if lib/kernel/* isn't a supported public import path, or turn the line-17 comment into a /** @deprecated Import from '../utils/assertBindableValue' instead. */ JSDoc on the export. The examples/ type-check errors you mentioned don't affect this either way.

[vuanhphung]

I think we can just remove this

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

I agree with removing the file, but I can't do it from here. My tools can edit or create files but not delete them, and rm / git rm aren't on my allowed command list. Leaving the file empty would only be a half-removal. I checked and nothing in lib/ or tests/ imports lib/kernel/KernelInputValidation.ts: KernelPositionalParams.ts and the unit test both import ../utils/assertBindableValue directly. The only remaining match is the test's description string, 'KernelInputValidation.assertBindableValue' in tests/unit/kernel/inputValidation.test.ts, which a human may want to rename. To finish, a human should run git rm lib/kernel/KernelInputValidation.ts and then npm test to confirm the suite still passes. One thing to weigh first: the file's JSDoc says it was kept because the package has no exports map, so dist/kernel/KernelInputValidation is a deep-import path consumers can reach. Deleting it would break anyone importing that path directly.

[peco-review-bot]

Verdict: 1 Low · 1 Nit

Looks good: running every value through a shared assertBindableValue gate before the Thrift RPC fixes the hang from #261 cleanly. The check runs inside the async executeStatement, so bad values reject the call (no synchronous throw) and the driver is never called. The CHANGELOG documents the behavior changes on both backends. I found only two low-severity test issues: the deprecated shim has no test coverage, and one e2e test name is misleading. Nit: the shim could be written as a plain re-export (export { default } from '../utils/assertBindableValue') instead of a wrapper function.

Other findings

• ⚪ Nit — The test is called should support multi-row inserts with flattened scalar ordinal parameters, but it only runs SELECT * FROM VALUES (?), (?), (?). No INSERT runs, so the workaround the test name points users to for #261 (one flattened ? per value in a multi-row INSERT ... VALUES) is never tested end-to-end. Either rename the test to describe what it actually checks (multi-row VALUES binding), or create a temp table, run INSERT INTO t VALUES (?), (?), (?) with [1, 2, 3], and read the rows back.

[peco-review-bot]

✅ No issues identified by the review bot.