Repository navigation
feat(csharp): support native SEA metadata results - #692
Conversation
|
I tried to validate this live against a dogfood staging warehouse ( I sent I also ran an ad-hoc probe of 25 metadata calls through the driver, on this PR ( Questions:
|
|
Suggestion: decode native and SHOW results once, at the boundary Right now each consumer decodes both result formats inline. The PR adds about 100 Each consumer only needs the same few fields per row, whatever format the server sent. Small row types plus one decoder per operation would hide the native/SHOW difference: internal readonly record struct SchemaRow(string Catalog, string Schema);
internal readonly record struct TableRow(string Catalog, string Schema, string Table, string TableType);
internal readonly record struct ColumnRow(
string Catalog, string Schema, string Table, string Name, string TypeName,
bool Nullable, int Ordinal /* one convention for both formats */, string? Default, bool IsAutoIncrement);
internal static class MetadataRowReader
{
// Picks the decoder once per result; consumers never see IsNative.
internal static IEnumerable<ColumnRow> Columns(MetadataBatches result, string? sourceCatalog)
=> result.Batches.SelectMany(b => result.IsNative ? NativeColumns(b, sourceCatalog) : ShowColumns(b));
private static IEnumerable<ColumnRow> NativeColumns(RecordBatch batch, string? sourceCatalog)
{
var c = new NativeMetadataColumns(batch, MetadataSchemaFactory.CreateColumnMetadataSchema(), MetadataOperation.GetColumns);
for (int i = 0; i < batch.Length; i++)
{
string? cat = c.String("TABLE_CAT", i) ?? sourceCatalog;
// Native GetColumns treats the catalog as a LIKE pattern; keep only the exact catalog.
if (sourceCatalog != null && !string.Equals(cat, sourceCatalog, StringComparison.OrdinalIgnoreCase)) continue;
// ... read schema/table/name/type, normalize ORDINAL_POSITION ...
yield return new ColumnRow(/* ... */);
}
}
private static IEnumerable<ColumnRow> ShowColumns(RecordBatch batch) { /* the existing SHOW COLUMNS parsing from main */ }
// Same pattern for Catalogs / Schemas / Tables.
}With that, consumers go back to roughly what they look like on var fields = ReadColumns(resolvedCatalog, dbSchema, tableName, null, cts.Token)
.Select(c => new Field(c.Name, ToArrowType(c.TypeName), c.Nullable))
.ToList();
What else this removes or fixes:
One fork remains. For flat results, |
|
Follow-up after checking the open questions live, and with a unit test against a mocked native response: 1. GetObjects with a wildcard catalog drops every table on native results (confirmed). I mocked native results:
The native branch of 2. Correction: the 3. 4. Native mode is still not testable on dogfood. I tried six warehouses: two Reyden, three PRO serverless and one classic PRO. All of them ignore |
65bd2ee to
3d7ea2a
Compare
|
🔒 Integration test approval reset New commits were pushed to this PR. The A maintainer must re-review the changes and re-add the label to trigger tests again. Why is this necessary?
Latest commit: 3d7ea2a |
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested feature: native SEA metadata decoding with careful alignment to existing flat/GetObjects behavior (ordinals, type sizes, table-type defaults, catalog filtering), and good unit coverage of headers, polling, result shaping, and catalog patterns. Schema indices, BASE_TYPE_NAME handling, manifest-driven decoder selection, and the catalog LIKE-pattern resolution all check out. One medium concern: the pre-existing bare catch { continue; } in the column fanout now also wraps native schema validation, silently dropping a catalog's columns on a malformed native result (and still swallows cancellation).
There was a problem hiding this comment.
Verdict: 1 Low
Looks good overall — one low-severity concern about an over-broad catch in the GetColumns catalog fanout. The native-vs-SHOW decoder selection keyed on the response manifest (including the poll-then-reread path), the Thrift-schema field ordering used for sorting/filtering, the catalog pattern resolution/slicing, and the flat-vs-GetObjects ordinal bases all verified consistent with MetadataSchemaDefinitions and existing parity behavior, and are well covered by the new unit tests.
| catch | ||
| { | ||
| // Skip catalogs we can't access (permission errors) | ||
| continue; |
There was a problem hiding this comment.
🔵 Low — The parameterless catch { ... continue; } around the per-catalog ExecuteNativeMetadataSqlAsync call in the all-catalogs GetColumns fanout swallows every exception, not just the permission errors the comment describes. In particular it also catches OperationCanceledException and the metadata TimeoutException produced by the surrounding CreateMetadataTimeoutCts() path, so a cancelled or timed-out GetColumns silently skips the remaining catalogs and returns a partial result instead of propagating. Consider narrowing the catch (e.g. rethrow when cancellationToken.IsCancellationRequested / the exception is OperationCanceledException/TimeoutException, and only swallow access errors). This behavior pre-existed in ExecuteShowColumnsAsync, but the block was restructured here.
(Anchored to the nearest changed line — see the description for the exact location.)
There was a problem hiding this comment.
Verdict: 1 Low
Looks good overall — a thorough, well-tested addition of native-SEA metadata decoding with a clean decoder-selection-via-manifest design, and the field-index assumptions all check out against MetadataSchemaFactory. One low-severity note: the rewritten ReadColumnsAsync catalog fan-out uses a bare catch that also swallows cancellation. The empty-vs-null tableTypes parity, zero-based flat / +1 GetObjects ordinal handling, and the polling-manifest fallback for IsNativeMetadataResult are all correct and covered by tests.
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Solid, well-tested implementation of native SEA metadata — flat/GetObjects ordinal, catalog-pattern, type-size, and validation behavior are consistently handled and the native-vs-manifest selection logic (including the post-polling flag precedence) is correct. One medium concern: GetTableSchema on the native path doesn't constrain column rows to the requested catalog, so native LIKE-expansion of a catalog name can merge a sibling catalog's columns into the returned schema (the flat GetColumns / PopulateColumnInfo paths guard this but GetTableSchema does not). Plus a low note on a broad catch swallowing cancellation during the per-catalog column fanout.
There was a problem hiding this comment.
Verdict: 1 Low
Looks solid — one low-severity error-handling note. The native/SHOW decoder selection, polling-manifest precedence, 0-based/+1 ordinal handling, catalog-pattern resolution, and empty-vs-null tableTypes semantics all check out against the surrounding code and the repo's documented parity rules, and the test coverage is thorough. The only concern is a bare catch in the all-catalogs column fan-out that swallows cancellation/timeout along with permission errors (F1).
| result = await ExecuteMetadataCommandAsync( | ||
| sql, MetadataOperation.GetColumns, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch |
There was a problem hiding this comment.
🔵 Low — The all-catalogs column fan-out uses a bare catch { continue; } intended only for per-catalog permission errors, but it also swallows OperationCanceledException / TimeoutException. When the caller's metadata-timeout CTS (e.g. GetTableSchema / GetColumnsAsync via CreateMetadataTimeoutCts) fires mid-fan-out, every remaining ExecuteMetadataCommandAsync throws immediately from ThrowIfCancellationRequested, each gets swallowed, and the loop runs to exhaustion — so ReadColumnsAsync returns a partial ColumnMetadataResult instead of propagating the timeout. The outer catch when (cts.IsCancellationRequested ...) guards in the callers never see an exception, so a timed-out GetColumns silently yields truncated metadata rather than erroring.
This mirrors the pre-existing catch {} in the old ExecuteShowColumnsAsync, so it's not a new regression, but since this block was rewritten it's worth narrowing: rethrow when cancellationToken.IsCancellationRequested (or catch only DatabricksException) so genuine cancellation/timeout surfaces.
|
🚀 Integration tests triggered! View workflow run |
|
🚀 Integration tests triggered! View workflow run |
There was a problem hiding this comment.
Verdict: 1 Low
Solid, well-tested PR — the native vs SHOW decoder selection, catalog filtering, table-type normalization, ordinal conventions (verbatim flat / +1 GetObjects, matching the Thrift base's PositionRequiredOffset), empty-tableTypes match-none, and native-schema validation all check out, and the unit/E2E coverage is thorough. Only one minor, pre-existing concern: the catalog fanout's bare catch swallows cancellation/timeout alongside permission errors.
| catch | ||
| { | ||
| // Skip catalogs we can't access (permission errors) | ||
| continue; |
There was a problem hiding this comment.
🔵 Low — The per-catalog fanout in ReadColumnsAsync (the catalog == null branch) wraps ExecuteMetadataCommandAsync in a bare catch { continue; } whose comment scopes it to "permission errors". It actually swallows every exception, including OperationCanceledException. When the caller's cancellationToken fires (e.g. the metadata timeout CTS set up in GetTableSchema / PopulateColumnInfoAsync for a null catalog), each remaining catalog iteration throws OperationCanceledException from ThrowIfCancellationRequested, gets swallowed, and the loop quietly finishes — so cancellation/timeout returns a partial-or-empty ColumnMetadataResult instead of propagating. The single-catalog path converts cancellation into a TimeoutException, so the two paths diverge on timeout behavior.
This predates the PR, but the block was rewritten here, so it's a good opportunity to narrow the catch (e.g. catch (Exception ex) when (ex is not OperationCanceledException)), so genuine cancellation/timeout surfaces while still skipping inaccessible catalogs.
|
🔒 Integration test approval reset New commits were pushed to this PR. The A maintainer must re-review the changes and re-add the label to trigger tests again. Why is this necessary?
Latest commit: dca1b58 |
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a sizeable but well-tested, comparator-validated refactor adding Thrift-native SEA metadata. I verified the trickier bits: the native-flag poll logic correctly prefers the final manifest and falls back to the initial (response is reassigned at line 443); SHOW/native ordinal bases are internally consistent (both 0-based, GetObjects adds +1 to both, flat passes both through); and NativeMetadataColumns validation is order-independent and strict. One low-severity error-handling note on the catalog-fanout catch (swallows cancellation/timeout silently).
|
🔒 Integration test approval reset New commits were pushed to this PR. The A maintainer must re-review the changes and re-add the label to trigger tests again. Why is this necessary?
Latest commit: 92db613 |
There was a problem hiding this comment.
Verdict: 1 Low
Looks solid overall — a well-factored unification of the SEA metadata paths (native-vs-SHOW decoder selection driven by the response manifest) with genuinely thorough unit coverage and a comparator run (208→4 diffs) backing the behavior. I verified the points most likely to hide bugs: KEQ_SEQ is a pre-existing, cross-path-consistent field name (not a typo to fix here); the NormalizePrecisionAndScale refactor is a no-op for the SHOW/flat path and only normalizes server-supplied native values; ordinal handling (0-based rows, GetObjects +1, skipped incomplete rows) matches prior behavior and tests; and native-schema validation errors correctly propagate rather than being swallowed by the fanout catch. One low-severity robustness note on that fanout catch swallowing cancellation/timeout (posted inline).
| sql, MetadataOperation.GetColumns, cancellationToken, | ||
| sourceCatalog, CatalogFilter.Exact(sourceCatalog)).ConfigureAwait(false); | ||
| } | ||
| catch |
There was a problem hiding this comment.
🔵 Low — The all-catalogs ReadColumnsAsync fanout wraps each per-catalog ExecuteMetadataCommandAsync in a parameterless catch { continue; }. This swallows every exception type, including OperationCanceledException/TimeoutException raised when the caller's token is cancelled or the metadata-timeout CTS fires.
Concrete scenario: GetTableSchema(null, ...) and GetObjects column population run under CreateMetadataTimeoutCts(). If the timeout trips mid-fanout, each remaining catalog's ExecuteMetadataCommandAsync throws a cancellation exception, which is caught and skipped, so the loop silently churns through the rest of the catalog list and returns a partial/empty ColumnMetadataResult instead of surfacing the timeout. The outer catch (... when cts.IsCancellationRequested ...) in GetTableSchema never sees a TimeoutException to translate.
This is the same bare-catch pattern as the prior ExecuteShowColumnsAsync, so it is not a new regression — but since the hunk was rewritten here it's worth tightening: rethrow cancellation (e.g. catch (OperationCanceledException) → throw; before the permission-swallowing catch) so timeouts/cancellation propagate while genuine per-catalog permission errors are still skipped.
|
🚀 Integration tests triggered! View workflow run |
|
🚀 Integration tests triggered! View workflow run |
All DBSQL warehouses on 19.x should have this behavior if the request comes from our drivers |
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
|
🔒 Integration test approval reset New commits were pushed to this PR. The A maintainer must re-review the changes and re-add the label to trigger tests again. Why is this necessary?
Latest commit: f0ccb1d |
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a large, well-structured feature with strong unit/E2E coverage and a comparator run showing near-parity (4 residual diffs). Native-vs-SHOW decoding is manifest-selected, catalog Exact/Pattern filtering is used deliberately and tested against underscore/percent catalog names, the precision/scale refactor preserves existing SHOW-path output, and the KEQ_SEQ field name intentionally matches the upstream typo in the shared schema definitions. Only one low-severity efficiency note (per-row regex rebuild in native catalog filtering); no correctness or contract concerns found.
| !MatchesParent(parentCatalog, parentSchema, parentTable, | ||
| row.String("PKTABLE_CAT"), row.String("PKTABLE_SCHEM"), row.String("PKTABLE_NAME"))) | ||
| continue; | ||
| rows.Add(row); |
There was a problem hiding this comment.
🔵 Low — When a native result is filtered by a wildcard CatalogFilter, this per-row MatchesCatalog call reaches MatchesCatalogPattern, which rebuilds the glob→regex string from scratch (a fresh StringBuilder scan over filter.Value) and calls Regex.IsMatch on every single row. The pattern depends only on filter.Value, which is constant for the whole result, so for a large GetColumns/GetTables native batch this repeats identical regex construction thousands of times.
.NET's static Regex.IsMatch cache (default size 15, keyed on pattern+options) blunts the actual compile cost, so this is minor — but the per-row StringBuilder rebuild is pure redundant work. Consider translating the pattern to a Regex (or a precomputed match delegate) once per MetadataBatches/CatalogFilter and reusing it across rows. Not a correctness issue; purely an efficiency cleanup.
|
🚀 Integration tests triggered! View workflow run |
2 similar comments
|
🚀 Integration tests triggered! View workflow run |
|
🚀 Integration tests triggered! View workflow run |
|
@eric-wang-1990 confirm latest E2E tests are passing with comparator runs not showing any new diff https://databricks.slack.com/archives/C0B70MDS8DP/p1791560007960709. |
|
Non-blocking, proposal for a follow-up PR after this merges: split the metadata code by result format Thanks for the updates, all my inline points are addressed in
Proposed layout (folders map to namespaces, like Each folder's class is a // Metadata/Native/Tables.cs (Metadata/Legacy/Tables.cs mirrors it)
namespace AdbcDrivers.Databricks.StatementExecution.Metadata.Native
{
internal sealed partial class MetadataDecoder : IMetadataDecoder
{
public IEnumerable<TableRow> Tables(MetadataBatches result) { /* Thrift-shaped columns */ }
}
}
// Metadata/MetadataRowReader.cs: the only place that branches on the format
IMetadataDecoder decoder = result.IsNative ? Native.MetadataDecoder.Instance : Legacy.MetadataDecoder.Instance;What goes where:
With the result builders mirrored, the statement's Tests: one parity Happy to help with or review the follow-up. |
|
@eric-wang-1990 sure I'll follow up on that after this PR. Is this fine to merge now? |
Request Thrift-native SEA results for C# catalog, schema, table, column, primary-key, and cross-reference metadata. Select native versus SHOW parsing from the response manifest, and always send native metadata headers for supported SEA requests.
Align native results with existing C# metadata behavior: flat and GetObjects ordinals, type sizes, table types, defaults, and auto-increment values. Validate native schemas and preserve literal catalog filtering. Unit tests cover request headers, polling, result shaping, and catalog filters.
Comparator run 36931597557 tested
8ac8ebc: 8,174 matches and 4 diffs, down from 208 in the prior run. The four remaining cases are unchanged (three table-type filters and one procedure result). C# CI, lint, and Thrift/REST E2E checks passed on the preceding commit.