Repository navigation
Return empty primary-key metadata for missing objects in Thrift - #553
vuanhphung wants to merge 5 commits into
Conversation
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| public async getSchema(options?: GetSchemaOptions): Promise<TTableSchema | null> { | ||
| return this.withErrorTelemetry(async () => { | ||
| await this.failIfClosed(); | ||
| await this.waitUntilReadyThroughBackend(options); |
There was a problem hiding this comment.
🔵 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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| error.sqlState === '42704' || | ||
| [error.errorMessage, error.displayMessage].some((message) => | ||
| ['NO_SUCH_CATALOG_EXCEPTION', 'SCHEMA_NOT_FOUND', 'TABLE_OR_VIEW_NOT_FOUND', 'INVALID_PARAMETER_VALUE'].some( | ||
| (errorClass) => message?.includes(errorClass), |
There was a problem hiding this comment.
🔵 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.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
74797df to
db5964d
Compare
There was a problem hiding this comment.
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.
| // Match Databricks JDBC's metadata not-found classification. | ||
| return ( | ||
| error.sqlState === '42704' || | ||
| [error.errorMessage, error.displayMessage].some((message) => |
There was a problem hiding this comment.
🔵 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 includesINVALID_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.
|
|
||
| export default function createPrimaryKeysSchema(): TTableSchema { | ||
| return { | ||
| columns: ['TABLE_CAT', 'TABLE_SCHEM', 'TABLE_NAME', 'COLUMN_NAME', 'KEY_SEQ', 'PK_NAME'].map( |
There was a problem hiding this comment.
Should this be KEQ_SEQ instead of KEY_SEQ? I believe the thrift server sends KEQ_SEQ, but double check which one it should be.
| response.status.statusCode !== TStatusCode.INVALID_HANDLE_STATUS && | ||
| (isObjectNotFound(response) || isObjectNotFound(response.status)) | ||
| ) { | ||
| this.metadata = synthesizeThriftResultSetMetadata({ |
There was a problem hiding this comment.
Nit: Consider adding a debug log with the original SQLSTATE and message before returning the empty result
| import config from './utils/config'; | ||
|
|
||
| describe('Thrift primary-key metadata', () => { | ||
| const missingSchema = `missing_primary_keys_${v4().replace(/-/g, '')}`; |
There was a problem hiding this comment.
Optional: add a missing catalog e2e test case
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
pecotestingin 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.