SEA metadata: throw for invalid null/empty params (Thrift parity)#1390
Merged
gopalldb merged 9 commits intodatabricks:mainfrom Apr 9, 2026
Merged
SEA metadata: throw for invalid null/empty params (Thrift parity)#1390gopalldb merged 9 commits intodatabricks:mainfrom
gopalldb merged 9 commits intodatabricks:mainfrom
Conversation
… Thrift parity Change key-based SEA metadata operations (getPrimaryKeys, getImportedKeys, getExportedKeys, getCrossReference) to throw SQLException instead of returning empty ResultSets for invalid parameter combinations like null/empty table or null schema with explicit catalog. This matches Thrift server behavior. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]>
gopalldb
reviewed
Apr 8, 2026
| if (parentTable == null && foreignTable == null) { | ||
| boolean parentTableMissing = parentTable == null || parentTable.isEmpty(); | ||
| boolean foreignTableMissing = foreignTable == null || foreignTable.isEmpty(); | ||
| if (parentTableMissing && foreignTableMissing) { |
Collaborator
Author
There was a problem hiding this comment.
Done — added LOGGER.debug() before all throw sites in the latest commits.
gopalldb
reviewed
Apr 8, 2026
| IDatabricksSession session, String catalog, String schema, String table) throws SQLException { | ||
| LOGGER.debug("public ResultSet listExportedKeys() using SDK"); | ||
|
|
||
| if (table == null || table.isEmpty()) { |
Collaborator
Author
There was a problem hiding this comment.
Done — added LOGGER.debug() before the throw.
gopalldb
reviewed
Apr 8, 2026
| String catalog, String schema, String table, IDatabricksSession session) throws SQLException { | ||
| if (table == null) { | ||
| return null; | ||
| if (table == null || table.isEmpty()) { |
Collaborator
Author
There was a problem hiding this comment.
Done — added LOGGER.debug() with the table value before throwing.
gopalldb
reviewed
Apr 8, 2026
| } | ||
| } else if (schema == null) { | ||
| return null; | ||
| } else if (schema == null || schema.isEmpty()) { |
Collaborator
Author
There was a problem hiding this comment.
Done — added LOGGER.debug() that includes the explicit catalog value for context.
gopalldb
reviewed
Apr 8, 2026
|
|
||
| if (catalog == null || schema == null) { | ||
| return null; | ||
| throw new DatabricksSQLException( |
Collaborator
There was a problem hiding this comment.
why this exception? this will throw when catalog= null, but schema is valid
Collaborator
Author
There was a problem hiding this comment.
This is a safety net for when getCurrentCatalogAndSchema() returns null values (server error edge case). In normal flow: if catalog was null it gets resolved at line 533, and if schema was also null it gets resolved at line 536 — so both are non-null by this point. This check only triggers if the server returns null for current_catalog or current_schema. Added a comment and debug log with the actual values for clarity.
Address PR review comments: add LOGGER.debug() calls before each throw site in getCrossReference empty-string check, listExportedKeys null table check, and resolveKeyBasedParams validation. Also add comment clarifying the safety-net check for getCurrentCatalogAndSchema returning null values. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]>
Empty strings are passed through to the server like Thrift does, instead of being treated as invalid. Only null values trigger exceptions for table and schema parameters. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]>
Match Thrift behavior for getCrossReference: - null table on either side = "unspecified", return empty ResultSet - empty string table = invalid, throw SQLException (Thrift server rejects these) Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]>
…rns empty Based on comparator results, align all key-based metadata ops with Thrift: - getPrimaryKeys/getImportedKeys/getExportedKeys: throw for null OR empty table - getCrossReference: throw for null/empty parentTable, throw for empty foreignTable, return empty ResultSet for null foreignTable (Thrift treats as "unspecified") Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]>
Thrift's getExportedKeys returns empty ResultSet for empty table name (it always returns empty in DBSQL). Match this behavior by only throwing for null, not empty string. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]>
Delete stub directories for tests that were renamed from *ReturnsEmpty to *Throws — these tests now throw before making any server call, so stubs are not needed. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <[email protected]>
gopalldb
approved these changes
Apr 9, 2026
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
2 participants
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.
Summary
getPrimaryKeys,getImportedKeys,getExportedKeys,getCrossReference) to throwSQLExceptioninstead of returning emptyResultSets for invalid parameter combinationsWhat changed
DatabricksMetadataQueryClient.resolveKeyBasedParams(): Now throwsDatabricksSQLExceptionfor invalid combos instead of returningnullDatabricksMetadataQueryClient.listExportedKeys(): Added null/empty table validationDatabricksDatabaseMetaData.getCrossReference(): Added empty string validation for both parent and foreign sidesDatabricksMetadataQueryClientTest: Updated tests to expect exceptions instead of empty resultsMetadataNullResolutionTests: Updated integration tests to verify exception-throwing behaviorThrift behavior reference
getPrimaryKeys(null, null, table)getPrimaryKeys(catalog, null, table)getPrimaryKeys(catalog, schema, null)getCrossReference(all empty strings)Test plan
DatabricksMetadataQueryClientTest— 57 tests passMetadataNullResolutionTests(requires WireMock stub re-recording)NO_CHANGELOG=true
This pull request was AI-assisted by Isaac.