fix(csharp): align GetCrossReference Thrift/SEA on parent identifiers - #645
Merged
Conversation
GetCrossReference diverged between the Thrift and SEA protocols on the parent (PK-side) identifiers, and neither path matched the JDBC reference driver: 1. Thrift threw on an empty-string parent catalog. TGetCrossReferenceReq is null-guarded (a null parent is omitted, so the foreign-only GetColumnsExtended reuse works), but an empty-string parent is SENT as ParentCatalogName="", and the server rejects it (SHOW FOREIGN KEYS IN CATALOG `` -> TABLE_OR_VIEW_NOT_FOUND / INVALID_PARAMETER_VALUE). JDBC Thrift (DatabricksThriftServiceClient .listCrossReferences) catches isObjectNotFoundException and returns empty; ADBC Thrift had no such catch. Fix: catch object-not-found in DatabricksStatement.GetCrossReferenceAsync -> empty result. IsObjectNotFoundException gains a static overload over AdbcException so it matches the HiveServer2Exception the Thrift path throws. 2. SEA ignored the parent identifiers entirely. SHOW FOREIGN KEYS is scoped to the FOREIGN table, so it returns FKs to every parent; the JDBC reference filters the rows by the requested parent (CrossReferenceKeysDatabricksResultSetAdapter .includeRow) but ADBC SEA did not — over-returning for a table with FKs to multiple parents. Fix: filter returned rows by any specified (non-null) parent catalog/schema/table in GetCrossReferenceAsyncNoThrow. A null parent means "no constraint" (preserves the GetColumnsExtended foreign-only reuse, which passes null); an empty-string parent matches only an empty row value, so it filters to empty. Both paths now return an empty result for an empty-string parent + valid foreign side, matching the JDBC spec semantics, and SEA no longer over-returns for multi-parent tables. Closes the get_cross_reference Thrift-vs-SEA comparator diff without a whitelist (supersedes driver-test PR #1249). Unit tests: static IsObjectNotFoundException over HiveServer2Exception (empty-parent error shapes + unrelated-error negative), and ParentMatches (null=no-filter, exact + case-insensitive match, empty-string and non-matching filter out). 974 unit tests pass. Co-authored-by: Isaac
eric-wang-1990
added
the
engineer-bot
![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 focused, well-tested Thrift/SEA parity fix for GetCrossReference. The static IsObjectNotFoundException(AdbcException) overload, the Thrift object-not-found catch, and the SEA ParentMatches filter all line up with the JDBC reference semantics, and the unit tests cover the key cases. One low-severity edge case noted inline around the null-parent-column fallback in the SEA filter.
Addresses: - #3762302992 at csharp/src/StatementExecution/StatementExecutionStatement.cs:1940 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 2 Low
Looks good — a focused, correct alignment of GetCrossReference across Thrift/SEA that matches the JDBC reference (verified CrossReferenceKeysDatabricksResultSetAdapter.includeRow), with solid unit coverage of the two new helpers. Two Low notes: the Thrift catch broadens object-not-found swallowing to the FK side, and the new filter/catch wiring is only covered indirectly (predicate + classifier), not end-to-end.
Addresses: - #3762330882 at csharp/src/StatementExecution/StatementExecutionStatement.cs:1957 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, JDBC-parity-aligned fix with strong unit coverage (predicate + end-to-end Http-seam tests). Verified the two GetCrossReferenceAsyncNoThrow call sites pass parents correctly (null for the GetColumnsExtended reuse, distinct parent/foreign options for the metadata path), that ShouldReturnEmptyPKFKResult does not short-circuit the empty-string-parent case, and that the static IsObjectNotFoundException overload + narrowly-guarded Thrift catch behave as described. One low note on null/absent server parent columns potentially over-filtering on SEA only.
eric-wang-1990
changed the title
msrathore-db
approved these changes
Aug 27, 2026
This was referenced Aug 27, 2026
4 tasks
birschick-bq
pushed a commit
to birschick-bq/databricks
that referenced
this pull request
Sep 2, 2026
…alog (adbc-drivers#660) ## Summary The SEA `GetCrossReference` path filters the `SHOW FOREIGN KEYS` result by the requested parent catalog/schema/table (added in adbc-drivers#645, mirroring JDBC's `CrossReferenceKeysDatabricksResultSetAdapter.includeRow`). But `_metadataCatalogName` is **seeded from the connection's default catalog** at statement construction, so for a **getImportedKeys-style** call (only the foreign table specified, no parent) the seeded catalog was applied as a parent filter and dropped every FK whose real parent lives in a different catalog. **Effect:** SEA `getImportedKeys` returned an **empty** result while Thrift returned the imported FK — the dominant divergence in the C# ADBC Thrift-vs-SEA comparator (**17** `get_cross_reference` diffs). Verified live: the server returns the FK row (real parent in `comparator_tests`), the SEA driver fetches it, then the parent filter compares the seeded `hive_metastore` against it and drops it (`refs.Count=0`). JDBC does **not** hit this: `getImportedKeys` goes through the filter-free `listImportedKeys`, and `listCrossReferences` only filters when the caller supplied a (required, non-null) parent table. ## Fix Pass the parent catalog to the filter **only when the caller explicitly set it** (`adbc.get_metadata.target_catalog`), tracked by a new `_metadataCatalogSet` flag. Parent schema/table are not seeded, so they already no-op when unset and are passed through unchanged — only the catalog needs the guard. `ParentMatches` and the row loop are untouched, so the explicit-parent `getCrossReference` path filters exactly as before. | Call | parent catalog used to filter | |------|-------------------------------| | getImportedKeys (no parent supplied) | none → returns the child's FKs (matches Thrift & JDBC `listImportedKeys`) | | getCrossReference (explicit parent) | the caller's catalog (unchanged from adbc-drivers#645) | ## Test Adds `GetCrossReference_ImportedKeys_DoesNotFilterBySeededCatalog`: mocks a `SHOW FOREIGN KEYS` row whose parent catalog (`prod_cat`) differs from the seeded catalog (`main`), with no parent supplied, and asserts the row survives. It **fails without this fix** (row dropped, RowCount 0) and passes with it. The existing filter tests missed the bug because their fixture's parent catalog was also `main`, coincidentally matching the seed. ## Test plan - [x] `dotnet build` clean - [x] Full unit suite: **981 passed, 0 failed** - [x] New regression test verified to fail without the fix and pass with it - [x] **Comparator re-run** (`comparator-csharp-adbc` / `run.sh --config thrift-vs-sea`) confirms the 17 `get_cross_reference` diffs collapse — pending (this is the verification adbc-drivers#645 omitted) ## Not covered here (separate diffs) - Empty-string parent catalog: Thrift throws while SEA returns empty (outcome diff) — needs the object-not-found alignment, tracked separately. - `EXPLAIN` `plan` column non-determinism — a comparator-config `ignore_columns` change, not a driver issue. This pull request and its description were written by Isaac.
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
GetCrossReferencediverged between ADBC's Thrift and SEA protocol paths in how they handle the parent (PK-side) identifiers. Surfaced by the ADBC C# Thrift-vs-SEA comparator (get_cross_referenceoutcome diff): with an empty-string parent + a valid foreign side (fk_child), Thrift threw while SEA returned rows.This aligns both paths to one non-throwing behavior for the parent identifiers — the same contract the driver's
DESC TABLE EXTENDEDmetadata path (GetColumnsExtended) already follows: it returns FK information without ever throwing over an absent parent. After the fix, both protocols behave identically:null/ absentGetColumnsExtended's one-sided reuse relies on)""Root causes & fixes
1. Thrift threw on an empty-string parent.
TGetCrossReferenceReqis null-guarded (HiveServer2Connection.cs): a null parent is omitted from the RPC (so the foreign-onlyGetColumnsExtendedreuse works), but an empty-string parent is sent asParentCatalogName="", which the server rejects (SHOW FOREIGN KEYS IN CATALOG \`→TABLE_OR_VIEW_NOT_FOUND/INVALID_PARAMETER_VALUE, verified live). → Catch object-not-found inDatabricksStatement.GetCrossReferenceAsync→ empty result, so the parent path never throws.IsObjectNotFoundExceptiongains a static overload overAdbcExceptionto match theHiveServer2Exceptionthe Thrift path throws (same pattern asIsDescTableExtendedUnsupported). The null-parent path (GetCrossReferenceAsForeignTableAsync, used byGetColumnsExtended`) is untouched.2. SEA ignored the parent identifiers entirely.
SHOW FOREIGN KEYSis scoped to the foreign table, returning FKs to every parent; ADBC SEA added every row without filtering by the requested parent — a latent over-return bug for any table with FKs to multiple parents (invisible on single-parent fixtures, exposed by the empty-string case where the correct result is "nothing").→ Filter the returned rows by any specified (non-null) parent catalog/schema/table in
GetCrossReferenceAsyncNoThrow. A null parent means "no constraint" (preserves theGetColumnsExtendedforeign-only reuse, which passes null on all three); an empty-string parent matches only an empty row value → filters to empty. The filter compares the raw server value, not the value-population fallback, so a null server column can't match a requested parent against itself.Net: both protocols are non-throwing and agree for every parent input, and SEA no longer over-returns for multi-parent tables.
Test Plan
IsObjectNotFoundExceptionstatic overload overHiveServer2Exception— empty-parent error shapes (TABLE_OR_VIEW_NOT_FOUND/42P01,INVALID_PARAMETER_VALUE) return true; unrelated (ACCESS_DENIED) returns false.ParentMatches— null = no filter (matches any, incl. empty row); exact + case-insensitive match; empty-string and non-matching parent filter out.GetColumnsExtended's one-sided (null-parent) cross-reference reuse still returns the table's FKs.get_cross_referenceoutcome diff collapses.This pull request and its description were written by Isaac.