Repository navigation
Return empty primary-key metadata for missing objects in Thrift #553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6af3207
e2764e8
bf3a1b4
a2ce211
db5964d
f480b61
90f3611
cb2b3a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { v4 } from 'uuid'; | ||
| import { TTableSchema } from '../../thrift/TCLIService_types'; | ||
| import IOperationBackend from '../contracts/IOperationBackend'; | ||
| import { OperationState, OperationStatus } from '../contracts/OperationStatus'; | ||
| import { ResultFormat, ResultMetadata } from '../contracts/ResultMetadata'; | ||
| import Status from '../dto/Status'; | ||
|
|
||
| export default class EmptyOperationBackend implements IOperationBackend { | ||
| public readonly id = v4(); | ||
|
|
||
| constructor(private readonly schema: TTableSchema) {} | ||
|
|
||
| public hasResultSet(): boolean { | ||
| return true; | ||
| } | ||
|
|
||
| public async fetchChunk(): Promise<Array<object>> { | ||
| return []; | ||
| } | ||
|
|
||
| public async hasMore(): Promise<boolean> { | ||
| return false; | ||
| } | ||
|
|
||
| public async waitUntilReady(): Promise<void> { | ||
| return undefined; | ||
| } | ||
|
|
||
| public async status(): Promise<OperationStatus> { | ||
| return { state: OperationState.Succeeded, hasResultSet: true }; | ||
| } | ||
|
|
||
| public async getResultMetadata(): Promise<ResultMetadata> { | ||
| return { schema: this.schema, resultFormat: ResultFormat.ColumnBased, isStagingOperation: false }; | ||
| } | ||
|
|
||
| public async cancel(): Promise<Status> { | ||
| return Status.success(); | ||
| } | ||
|
|
||
| public async close(): Promise<Status> { | ||
| return Status.success(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| interface MetadataError { | ||
| sqlState?: string | null; | ||
| errorMessage?: string | null; | ||
| displayMessage?: string | null; | ||
| } | ||
|
|
||
| export default function isObjectNotFound(error: MetadataError): boolean { | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low —
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
peco-review-bot[bot] marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium —
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:
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".
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is fine, to match jdbc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for confirming that JDBC parity is the goal, and the inline comment does say so. Matching JDBC explains why |
||
| ['NO_SUCH_CATALOG_EXCEPTION', 'SCHEMA_NOT_FOUND', 'TABLE_OR_VIEW_NOT_FOUND', 'INVALID_PARAMETER_VALUE'].some( | ||
|
vuanhphung marked this conversation as resolved.
vuanhphung marked this conversation as resolved.
vuanhphung marked this conversation as resolved.
|
||
| (errorClass) => message?.includes(errorClass), | ||
|
vuanhphung marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — The 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 |
||
| ), | ||
| ) | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import { TTableSchema, TTypeId } from '../../thrift/TCLIService_types'; | ||
|
|
||
| export default function createPrimaryKeysSchema(): TTableSchema { | ||
| return { | ||
| // Preserve the Thrift server's KEQ_SEQ spelling. | ||
| columns: ['TABLE_CAT', 'TABLE_SCHEM', 'TABLE_NAME', 'COLUMN_NAME', 'KEQ_SEQ', 'PK_NAME'].map( | ||
| (columnName, index) => ({ | ||
| columnName, | ||
| typeDesc: { | ||
| types: [{ primitiveEntry: { type: columnName === 'KEQ_SEQ' ? TTypeId.INT_TYPE : TTypeId.STRING_TYPE } }], | ||
| }, | ||
| position: index + 1, | ||
| comment: '', | ||
| }), | ||
| ), | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| import { expect } from 'chai'; | ||
| import { v4 } from 'uuid'; | ||
| import { DBSQLClient } from '../../lib'; | ||
| import IDBSQLSession from '../../lib/contracts/IDBSQLSession'; | ||
| import { TOperationState, TStatusCode, TTypeId } from '../../thrift/TCLIService_types'; | ||
| import config from './utils/config'; | ||
|
|
||
| describe('Thrift primary-key metadata', () => { | ||
| const missingName = `missing_primary_keys_${v4().replace(/-/g, '')}`; | ||
| let client: DBSQLClient; | ||
| let session: IDBSQLSession; | ||
|
|
||
| before(async () => { | ||
| client = new DBSQLClient(); | ||
| await client.connect({ host: config.host, path: config.path, token: config.token }); | ||
| session = await client.openSession({ initialCatalog: config.catalog, initialSchema: config.schema }); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| try { | ||
| await session?.close(); | ||
| } finally { | ||
| await client?.close(); | ||
| } | ||
| }); | ||
|
|
||
| [ | ||
| { missingObject: 'schema', catalogName: config.catalog, schemaName: missingName }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 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 |
||
| { missingObject: 'catalog', catalogName: missingName, schemaName: config.schema }, | ||
| { missingObject: 'table', catalogName: config.catalog, schemaName: config.schema }, | ||
| ].forEach(({ missingObject, catalogName, schemaName }) => { | ||
| [1000, null].forEach((maxRows) => { | ||
| it(`returns typed empty primary keys for a missing ${missingObject} with direct results ${ | ||
| maxRows === null ? 'off' : 'on' | ||
| }`, async () => { | ||
| const operation = await session.getPrimaryKeys({ | ||
| catalogName, | ||
| schemaName, | ||
| tableName: missingName, | ||
| maxRows, | ||
| }); | ||
| try { | ||
| const schema = await operation.getSchema(); | ||
| expect( | ||
| schema?.columns.map((column) => [ | ||
| column.columnName, | ||
| column.typeDesc.types[0].primitiveEntry?.type, | ||
| column.position, | ||
| ]), | ||
| ).to.deep.equal([ | ||
| ['TABLE_CAT', TTypeId.STRING_TYPE, 1], | ||
| ['TABLE_SCHEM', TTypeId.STRING_TYPE, 2], | ||
| ['TABLE_NAME', TTypeId.STRING_TYPE, 3], | ||
| ['COLUMN_NAME', TTypeId.STRING_TYPE, 4], | ||
| ['KEQ_SEQ', TTypeId.INT_TYPE, 5], | ||
| ['PK_NAME', TTypeId.STRING_TYPE, 6], | ||
| ]); | ||
| expect(await operation.fetchAll()).to.deep.equal([]); | ||
| expect(await operation.hasMoreRows()).to.be.false; | ||
| const status = await operation.status(); | ||
| expect(status.status.statusCode).to.equal(TStatusCode.SUCCESS_STATUS); | ||
| expect(status.operationState).to.equal(TOperationState.FINISHED_STATE); | ||
| } finally { | ||
| await operation.close(); | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.