[vuanhphung]

When getPrimaryKeys() resolves a missing catalog, schema, or table, Thrift can throw while JDBC and kernel return an empty result set. Apply JDBC's not-found classification only to primary-key metadata, returning a successful empty result with the standard six-column schema for initial, direct-result, and polling failures. Preserve unrelated errors and server-handle cleanup.

Validated with 1,467 passing unit tests (36 new regressions), build, type checking, lint, and formatting. Replayed all 22 cases from https://github.com/databricks/databricks-driver-test/actions/runs/39944116811 using pecotesting in Thrift and kernel modes, with direct results enabled and disabled: all return zero rows with matching column names, types, and positions. Also verified real primary-key rows, omitted-catalog resolution, and ordinary SQL errors.

---

This PR was created with GitHub MCP.

[peco-review-bot]

Verdict: 1 Low

Looks good — a well-scoped, carefully-tested change that mirrors JDBC/kernel by returning a typed empty primary-key result for missing objects across the initial/direct/poll paths while preserving unrelated errors and handle cleanup. Imports, the no-param-reassign reassignment (rule is off in .eslintrc), and the reordered metadata assignment all check out. One low note inline about the breadth of the INVALID_PARAMETER_VALUE substring match potentially masking genuine invalid-argument errors on getPrimaryKeys.

[peco-review-bot]

Verdict: 1 Low

Looks good — a well-structured, heavily-tested change. The recovery flow across the initial-error, direct-results, and poll paths is correct, the constructor metadata reordering is necessary and sound, invalid-handle errors are correctly excluded, and server-handle cleanup is preserved. One low-severity note on the breadth of the INVALID_PARAMETER_VALUE not-found classifier (deliberate JDBC parity, but can mask genuine client errors as empty results).

[peco-review-bot]

Verdict: 2 Low

Looks good — a well-scoped feature with unusually thorough test coverage (initial/direct/poll deliveries, handle cleanup, cancel, error preservation, cross-session schema isolation). The this.metadata assignment is correctly moved ahead of processOperationStatusResponse so the synthesized empty metadata isn't overwritten, and EmptyOperationBackend validly implements IOperationBackend (only the optional operationType/dataProvider omitted). Two low-severity notes: the DBSQLOperation wait-ordering change affects all operation types beyond primary keys, and the not-found substring classification is deliberately broad.

[peco-review-bot]

🔵 Low — The reordering of waitUntilReadyThroughBackend() to run before the hasResultSet() check here (and the matching reorders in fetchChunkInternal and hasMoreRows) changes observable behavior for every operation type, not just primary keys — which is broader than the PR's stated primary-keys-only scope.

Specifically, for a failed operation whose handle reports hasResultSet === false, the old code short-circuited and returned null (getSchema) / [] (fetchChunk) / delegated to hasMore() (hasMoreRows) without surfacing the error. With the wait moved first, these methods now wait to a terminal state and therefore throw OperationStateError for a failed op. This is arguably more correct (and the new tests/unit/thrift-backend/ThriftPrimaryKeysMetadata.test.ts 'preserves a terminal error observed before result access' case codifies it), but it is a public-API behavior change affecting all operations and is not called out in the PR description. Worth confirming no downstream caller relied on the old silent-null/[] behavior for failed/no-result operations.

(Anchored to the nearest changed line — see the description for the exact location.)

[peco-review-bot]

Verdict: 1 Low

Looks solid — the missing-object recovery is correct across the initial/direct/poll delivery paths, scoped to primary-key metadata only, with per-call schema isolation and preserved handle cleanup; coverage is thorough. One low concern: the not-found classification relies on broad free-text substring matching (INVALID_PARAMETER_VALUE, and .includes() catching wrapped messages like INSUFFICIENT_PERMISSIONS … SCHEMA_NOT_FOUND), which can mask genuine client/permission errors as empty results — worth confirming this precisely mirrors JDBC.

[peco-review-bot]

Verdict: 1 Low

Looks solid — the not-found recovery for getPrimaryKeys is correctly scoped, handle cleanup is preserved, and the 36 new unit cases plus e2e cover initial/direct/poll delivery, isolation, cancel/close, and unrelated-error preservation. One low concern: the INVALID_PARAMETER_VALUE token (and order-independent substring matching) in isObjectNotFound can silently turn malformed-request errors into empty results — worth confirming against the intended JDBC classification.

[peco-review-bot]

🔵 Low — The INVALID_PARAMETER_VALUE token is matched as a "missing object" and converted into a successful empty primary-key result. Unlike NO_SUCH_CATALOG_EXCEPTION / SCHEMA_NOT_FOUND / TABLE_OR_VIEW_NOT_FOUND, an invalid-parameter error does not imply the object is absent — it signals a malformed request. The test suite encodes this deliberately (e.g. [INVALID_PARAMETER_VALUE.LENGTH] Identifier is too long is expected to yield an empty result), so a caller that passes an over-length or otherwise invalid identifier will see "no primary keys" instead of an error, masking a client bug. The substring match is also order-independent (.includes), so any server message that merely quotes one of these tokens anywhere in its text will be reclassified as not-found.

This appears intentional (the comment claims JDBC parity), so this is a flag-for-confirmation rather than a clear defect: please confirm Databricks JDBC really classifies INVALID_PARAMETER_VALUE as not-found for getPrimaryKeys, and consider tightening to anchored/bracketed error-class matching if the goal is only to catch genuine missing-object classes.

[peco-review-bot]

Verdict: 1 Low

Looks good overall: the not-found → empty-result conversion is limited to getPrimaryKeys, preserves handle cleanup and prefetched close, and has thorough tests. One low concern: the substring-based not-found check is broad and can hide permission or invalid-parameter errors. Nit: createContext() in the unit test sets operationType = GET_FUNCTIONS for a primary-keys handle, which is misleading.

[peco-review-bot]

🔵 Low — isObjectNotFound checks whether the error-class tokens appear anywhere in errorMessage/displayMessage, so it also matches errors that aren't about a missing object. Two cases:

• Any INVALID_PARAMETER_VALUE* error is treated as not-found. That includes INVALID_PARAMETER_VALUE.LENGTH, which the tests explicitly cover.
• A permission error that only mentions a not-found class as a wrapped cause ([INSUFFICIENT_PERMISSIONS] Wrapped cause: SCHEMA_NOT_FOUND, also tested) becomes an empty result. The caller then can't tell "no primary keys" apart from "you lack access" or "your argument was rejected".

If this deliberately matches JDBC, add a short comment or link to the JDBC source for the exact rule, so later readers don't narrow it by mistake. Otherwise, consider matching only the leading [ERROR_CLASS] bracket rather than any substring, and leave permission errors alone.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Mostly solid. The recovery works for initial, direct-result and poll failures, the tests are thorough, and handle cleanup is kept. One medium concern: the not-found classifier matches substrings too broadly, so permission errors and invalid-parameter errors can come back as empty results, logged only at debug. Nit: the unit-test createContext sets operationType = GET_FUNCTIONS on a primary-keys handle, which is misleading but harmless.

[peco-review-bot]

🔵 Low — The PR says it covers a missing catalog, schema, or table, but the e2e matrix only tests a missing schema and a missing catalog. Every case passes tableName: 'missing_table' together with an already-missing parent, so the TABLE_OR_VIEW_NOT_FOUND path (existing catalog and schema, missing table) is never tested end-to-end. That case may come back through a different delivery path, e.g. a successful empty result or an error status. Add a case with catalogName: config.catalog, schemaName: config.schema, tableName: missingName.

[peco-review-bot]

Verdict: 1 Nit

Looks good, with one low finding. The fallback is limited to getPrimaryKeys. It covers the initial-error, direct-result and polling paths, and keeps invalid-handle, cancel, close and timeout behaviour intact. Moving the metadata assignment so the server's direct-result metadata can't overwrite the synthesized schema is correct. The unit and e2e coverage is thorough. The one finding is that the not-found check matches error text too loosely, so some permission and validation errors also come back as empty results.

Other findings

• ⚪ Nit — The not-found check is a plain substring match over errorMessage/displayMessage. That means any error whose text contains one of these tokens anywhere gets turned into an empty successful result. Two cases go beyond "object is missing":

• Validation errors: INVALID_PARAMETER_VALUE and its subclasses, such as .LENGTH for an identifier that is too long. These are caller mistakes, but they now come back as zero rows.

• Permission failures: an INSUFFICIENT_PERMISSIONS error (SQLSTATE 42501) whose message mentions SCHEMA_NOT_FOUND as a wrapped cause. A user who lacks access will see "no primary keys" instead of an auth error.

The tests show both cases are deliberate, to mirror JDBC. Because this masks errors, it's worth one or both of:

• Matching only the leading [ERROR_CLASS] token, or the bracketed class anywhere, rather than any substring.
• Raising the recovery log from debug to info/warn, so swallowed permission and validation errors can be seen without debug logging turned on.

If exact JDBC parity is required, a short comment noting that these masking cases are intentional would help future readers.

[peco-review-bot]

Verdict: 1 Medium

Looks good overall, with one medium concern: the not-found check is a loose substring match, so permission errors and invalid-parameter errors can come back as successful empty results. The constructor reordering (metadata set before the status is processed), the hasResultSet override and the handle cleanup paths all look correct, and the unit tests cover them well. Nit: the unit-test createContext sets operationType = GET_FUNCTIONS on a primary-keys handle, which reads as a copy-paste leftover.

[peco-review-bot]

🟡 Medium — isObjectNotFound checks whether the error-class token appears anywhere in errorMessage/displayMessage. It doesn't look only at the leading [ERROR_CLASS], and it ignores SQLSTATE. Because of that, errors that have nothing to do with a missing object also turn into a successful empty result:

• An INSUFFICIENT_PERMISSIONS (SQLSTATE 42501) error whose message happens to mention SCHEMA_NOT_FOUND in a wrapped cause. The new unit tests require this to return empty rows.
• Any INVALID_PARAMETER_VALUE.* subclass, for example .LENGTH (identifier too long). This is a caller input error, not a missing object.
• Any message that mentions one of these tokens as free text, such as a table or comment name that contains TABLE_OR_VIEW_NOT_FOUND.

As a result, a caller who lacks privileges or passes an invalid identifier gets "no primary keys" with SUCCESS status and no error, and can't tell that from a real empty result. The only trace is a debug-level log line. Matching JDBC may justify part of this. Still, consider:

• matching only the leading error class (e.g. /^\[(NO_SUCH_CATALOG_EXCEPTION|SCHEMA_NOT_FOUND|TABLE_OR_VIEW_NOT_FOUND|INVALID_PARAMETER_VALUE)(\]|\.)/), and
• keeping permission SQLSTATEs (42501/28000) out of this path.

If the wrapped-permission case really is meant to be swallowed, say so in the CHANGELOG entry, because it changes user-visible behavior beyond "missing catalog/schema/table".

[vuanhphung]

this is fine, to match jdbc

[peco-review-bot]

Thanks for confirming that JDBC parity is the goal, and the inline comment does say so. Matching JDBC explains why INVALID_PARAMETER_VALUE and the general not-found classes are in the list. It doesn't cover the cases with real downside: a permission error (42501/28000) gets turned into SUCCESS with empty rows just because a wrapped cause mentions SCHEMA_NOT_FOUND, and the new tests now lock that in as required behavior. If the JDBC driver really does substring-match without checking SQLSTATE, please do at least one of these: link the JDBC source in the comment so future readers can check it, add a permission-SQLSTATE guard, or note in the CHANGELOG that some permission and invalid-parameter errors now come back as empty results. I'm leaving this open so a maintainer can make the final call.