feat(csharp): validate exact-match args for SEA GetPrimaryKeys / GetCrossReference

[eric-wang-1990]

Summary

Adds client-side argument validation to the SEA metadata exact-match operations
GetPrimaryKeys and GetCrossReference, mirroring the JDBC reference driver's
resolveKeyBasedParams / listCrossReferences. This gives clean, deterministic
errors and avoids relying on the Thrift server's inconsistent (and in one case buggy)
handling of missing arguments.

Split out of #604 (which handles object-not-found → empty,
backslash escaping, and the exception-type pivot) because exact-match argument
validation is a distinct concern.

Verified behavior (live thrift vs rest, + JDBC reference source)

Legend: empty = 0-row result, no throw. throw = AdbcException.

GetPrimaryKeys(catalog, schema, table)

input	Thrift (measured)	JDBC SEA (resolveKeyBasedParams)	ADBC SEA (this PR)
all set, object exists	rows	rows	rows
all set, object missing	empty	empty (isObjectNotFound catch)	empty
table = null	throw 42000 "tableName may not be null"	throw "tableName may not be null"	throw 42000
catalog set, schema = null	throw 08000 "GET_FUNCTIONS assertion failed" (server bug)	throw "schema may not be null when catalog is specified"	throw 42000 (clean; matches JDBC intent)
catalog = null (schema/table set)	throw 08000 (server bug)	resolves from session, else throw	empty (no session default)

GetCrossReference(pk…, fk…)

input	Thrift (measured)	JDBC SEA (listCrossReferences)	ADBC SEA (this PR)
foreign side fully set, exists	rows	rows	rows
both parent table AND foreign table = null	throw 42000 "foreignTable and parentTableName are both null"	empty ("unspecified")	empty (matches JDBC SEA — see note)
≥1 table set, foreign table = null (e.g. parent table set)	empty	empty ("unspecified")	empty
foreign catalog set, foreign schema = null (fk table set)	throw 08000 "GET_FUNCTIONS assertion failed" (server bug)	throw "schema may not be null when catalog is specified"	throw 42000 (clean; matches JDBC intent)
PK/FK feature disabled	(n/a)	empty	empty (short-circuit before validation; symmetric with GetPrimaryKeys)
object missing	empty	empty (isObjectNotFound catch)	empty

Note on the both-null case: ADBC SEA follows the JDBC SEA reference here, not Thrift.
DatabricksMetadataQueryClient.listCrossReferences checks only the foreign table — a null
foreign table is "unspecified" and returns an empty result; it never inspects the parent table.
So both-tables-null returns empty. Measured against the live Thrift server, the same input
throws a clean 42000 ("foreignTable and parentTableName are both null"), so this is an
intentional Thrift-vs-SEA divergence, whitelisted in the comparator (databricks-driver-test#1047).
When only the foreign table is null but a parent table is set, both Thrift and SEA return empty.

Why validate client-side

1. Thrift's 08000 on null-schema is a server bug — it surfaces an internal
   GET_FUNCTIONS assertion failed (a connection-exception SQLSTATE) instead of a
   clean argument error. JDBC validates before the RPC and throws a clean error;
   this PR does the same so SEA doesn't depend on (or replicate) the buggy path.
2. Deterministic, no round-trip — the error is raised before issuing SHOW KEYS /
   SHOW FOREIGN KEYS.

Scope / correctness notes

• Validation lives only on the user-facing command path. Each exact-match op is a
  thin public wrapper that reads the statement fields, validates, and throws
  (GetPrimaryKeysAsync / GetCrossReferenceAsync), over a non-throwing internal
  core that takes the key parts as explicit params and returns empty for
  unspecified/invalid args (GetPrimaryKeysAsyncInternal(catalog, schema, table, …) /
  GetCrossReferenceAsyncInternal(pkCatalog, pkSchema, pkTable, fkCatalog, fkSchema, fkTable, …)).
  GetColumnsExtended's internal three-call fallback reuses the PK/FK fetch with a null
  schema on purpose (to gather keys for a column set); it calls the internals
  directly, so its legitimately-unspecified args return empty instead of being rejected.
  The wrappers throw only when the feature is engaged and the internal would otherwise
  reach a live fetch, so the two paths never diverge.
• Errors are DatabricksException (the SEA-natural type). The comparator compares
  Status + SqlState, not the concrete subclass, so this matches Thrift's path
  where Thrift returns a clean error, and is whitelisted where Thrift emits its
  08000 bug.

Comparator

The residual Thrift 08000-vs-SEA 42000 sqlstate diffs (foreign/catalog schema-null)
are documented as a Thrift server bug in databricks-driver-test's test_case_filters
(schema-null exact-match skip). The both-tables-null case is also whitelisted there
(databricks-driver-test#1047): ADBC SEA now returns empty (matching JDBC SEA) while
Thrift throws 42000 — an intentional Thrift-vs-SEA divergence.

All behavior in the tables above was verified against the live Thrift server via a
throwaway E2E probe (TGetPrimaryKeysReq / TGetCrossReferenceReq), not inferred from
code or mocks.

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Medium

Focused, well-tested change that mirrors the JDBC reference driver; validation placement and the internal validateArgs: false reuse are correct. One medium concern: the new throw in GetCrossReferenceAsync runs before the PKFK-disabled empty short-circuit, making it inconsistent with GetPrimaryKeysAsync when EnablePKFK == false.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Medium

Looks good — the argument-validation logic is self-consistent and well-tested (short-circuit ordering, validateArgs:false reuse path, and direct FetchCrossReferenceAsync bypass are all correct). One medium concern: the PR description's GetCrossReference table and "both-null" note claim ADBC SEA throws (follows Thrift, no comparator whitelist needed), but the code, inline comment, and tests all return empty (follow JDBC, requiring a whitelist) — reconcile the description and confirm the external comparator whitelist matches the shipped empty behavior.

Other findings

• 🟡 Medium — The PR description contradicts the shipped behavior for the both-tables-null GetCrossReference case, and the two descriptions imply opposite comparator-whitelist requirements.

• The description's GetCrossReference table and the "Note on the both-null case" both state ADBC SEA throws 42000 for both-null, "follows Thrift rather than JDBC," and that matching Thrift converges the comparator ("no whitelist needed").

• The actual code returns an empty result when the foreign table is null (regardless of the parent), the inline comment says it "follows JDBC SEA here, not Thrift ... the comparator whitelists that one input," and GetCrossReference_BothTablesNull_ReturnsEmpty asserts RowCount == 0.

So the shipped code follows JDBC (empty) and requires the comparator to whitelist the both-null input as a Thrift-vs-SEA divergence (Thrift throws, SEA returns empty). If any comparator/whitelist configuration (outside this diff) was set per the description's claim that the case "converges" and needs no whitelist, the comparator will fail on the both-null input. Please reconcile the PR description with the implementation and confirm the comparator whitelist matches the shipped empty behavior.

[eric-wang-1990]

Instead of passing in this extra validateArgs param, we should change the callsite where we do not expect it to throw, to return empty rather than throw.

[peco-engineer-bot]

The change is complete and clean. I've done exactly what the reviewer asked.

Pushed 252963e.

[peco-review-bot]

Verdict: 1 Low

Looks good — the client-side exact-match validation mirrors the JDBC reference driver, is symmetric between GetPrimaryKeys/GetCrossReference, correctly short-circuits the disabled-PKFK and columns-extended reuse paths before validation, and throws are placed before the object-not-found try/catch so they aren't swallowed to empty. Tests cover the throw and empty cases well. One low note: an empty-result guard in GetPrimaryKeysAsync is now unreachable given the new throws.

[peco-review-bot]

✅ No issues identified by the review bot.

[eric-wang-1990]

Should this be changed to try catch pattern? So that if there are other throw scenario in the future we do not break this return empty thing?

[peco-engineer-bot]

Kept the explicit pre-check guard rather than switching to try/catch. A try/catch fallback to empty would have to catch DatabricksException broadly, since NewInvalidArgumentException (line 1131) produces the same AdbcStatusCode.InternalError/SqlState 42000 shape as a genuine server-side error — so it would silently swallow real execution failures (network/auth/server) and drop PK data from the columns-extended result instead of surfacing them. object-not-found is already handled inside GetPrimaryKeysAsync (line 1703). The guard's two conditions exactly mirror the method's two client-side validations, and the coupling is documented on both sides (comment at lines 1661–1664), which mitigates the drift concern the reviewer is worried about. No code change warranted.

[eric-wang-1990]

Refactored (2e5fd7d) per the suggestion: instead of the caller-side pre-check guard (or the earlier validateArgs flag), each exact-match op is now a thin public wrapper that validates + throws, delegating to a non-throwing internal core:

• GetPrimaryKeysAsync / GetCrossReferenceAsync (public commands) validate the exact-match args and throw a clean 42000, then call the internal.
• GetPrimaryKeysAsyncInternal / GetCrossReferenceAsyncInternal do the fetch and return empty for unspecified/invalid args (never throw). GetColumnsExtendedViaThreeCalls calls these directly, so its legitimately-null schema is never rejected.

This avoids the try/catch (which would have to catch DatabricksException/InternalError/42000 broadly and could swallow a real server error), and it's future-proof: any new validation added to a public wrapper stays out of the internal reuse path by construction. Verified behavior-identical to the guard version across all catalog/schema/table combinations; 240 StatementExecution unit tests pass.

[peco-engineer-bot]

⛔ NEEDS HUMAN DECISION — the bots can't resolve this thread; a maintainer's input is required.

Thread has converged: the reviewer implemented their own suggestion in commit 2e5fd7d (public validating wrappers GetPrimaryKeysAsync/GetCrossReferenceAsync delegating to non-throwing internals GetPrimaryKeysAsyncInternal/GetCrossReferenceAsyncInternal; GetColumnsExtendedViaThreeCalls now calls the internal + FetchCrossReferenceAsync directly). No edit needed from me — I verified the current tree builds clean (0 warnings/0 errors) and StatementExecution unit tests pass (240 passed, 26 skipped E2E, 0 failed). The refactor is future-proof (new validations on public wrappers stay out of the internal reuse path) and avoids the broad-catch try/catch that could swallow real server errors. Nothing further to action here.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Low

Looks good — the wrapper/internal split is sound. The validation guards mirror the internal early-returns exactly (ShouldReturnEmptyPKFKResult + unspecified-arg checks), so throws only replace would-be-empty results and never drop rows; the PK/FK-disabled short-circuit is symmetric across both ops, GetColumnsExtendedViaThreeCalls correctly routes to the non-throwing internals, and the tests cover each documented case. One low-severity telemetry note inline.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

Verdict: 1 Low

Looks good — the split into throwing public wrappers (GetPrimaryKeysAsync / GetCrossReferenceAsync) over non-throwing internals correctly mirrors the JDBC reference driver, the renamed FetchCrossReferenceAsync → GetCrossReferenceAsyncInternal has no dangling callers, and the tests cover both the throw and empty paths. One low telemetry note: GetPrimaryKeysAsync's guard fires whenever the feature is engaged (not only on rejection), so valid calls emit a redundant empty span.

[peco-review-bot]

Verdict: 1 Low

Looks good — the exact-match validation is carefully structured and well-tested. Verified that the sync TraceActivity rethrows (so the wrapper's throw isn't swallowed), that the willReject guard is equivalent to the two inner throw conditions (no fall-through to a spurious empty), and that ShouldReturnEmptyPKFKResult usage is consistent between wrapper and internal. One low-severity maintainability note about the duplicated precondition logic between the wrappers and their internals.

[eric-wang-1990]

MetadataUtilities.ShouldReturnEmptyPKFKResult should be the first check to return empty. Then you can also skip the same check inside GetPrimaryKeysAsyncInternal

[peco-engineer-bot]

That's fine — no live agent; the reply goes through the structured outcome report.

I addressed the reviewer's comment. Summary:

What the comment asked: Make MetadataUtilities.ShouldReturnEmptyPKFKResult the first check to return empty, and then also skip the same check inside GetPrimaryKeysAsyncInternal.

What I did:

1. First half (applied): Restructured GetPrimaryKeysAsync so ShouldReturnEmptyPKFKResult is the first statement and returns an empty result on true, replacing the awkward willReject double-negative guard with a plain early-return. Arg validation now only runs once the feature is genuinely engaged.
2. Second half (did not apply, with reason): Keeping the check inside GetPrimaryKeysAsyncInternal because that method is a second entry point — `GetColu

Pushed bca2fd1.

[peco-review-bot]

Verdict: 1 Low

Looks good — the client-side arg validation, wrapper/internal split, and non-throwing core all line up with the JDBC-parity intent, and the tests cover the throw/empty matrix well. One low-severity note: the GetPrimaryKeys disabled-feature/invalid-catalog short-circuit no longer emits a telemetry span, an asymmetry with the GetCrossReference path which still does.

[peco-review-bot]

Verdict: 1 Low

Looks good — a clean, well-reasoned refactor splitting each exact-match op into a validating public wrapper over a non-throwing internal, with the guard conditions correctly mirroring the internal early-returns (verified ShouldReturnEmptyPKFKResult semantics and the synchronous TraceActivity rethrow behavior). One low-severity note: the non-throwing internal-reuse path (columns-extended with a null schema) is only covered by CI-skipped E2E tests, so consider adding a unit guard. Nit: the GetCrossReferenceAsync throw-path span sets only the fk_* tags and omits the pk_* tags the internal sets — harmless telemetry asymmetry.

[peco-review-bot]

Verdict: 1 Low

Looks good — the wrapper/internal split is sound: sync TraceActivity rethrows so client-side validation throws propagate, and the throw-guards mirror the non-throwing cores' early-returns via the same ShouldReturnEmptyPKFKResult call, so they can't diverge (disabled feature / invalid catalog short-circuits to empty before any throw). The columns-extended reuse is behavior-preserving and newly test-guarded. One low: a stale method-name reference in a test comment.

[peco-review-bot]

✅ No issues identified by the review bot.

[peco-review-bot]

✅ No issues identified by the review bot.