Skip to content

feat(csharp): support native SEA metadata results - #692

Merged
vuanhphung merged 22 commits into
mainfrom
vu-phung/csharp-native-metadata
Oct 12, 2026
Merged

vuanhphung merged 22 commits into
mainfrom
vu-phung/csharp-native-metadata

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

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.

@vuanhphung vuanhphung added ai-assisted integration-test Trigger integration tests in internal repo labels Oct 1, 2026
@eric-wang-1990

Copy link
Copy Markdown
Collaborator

I tried to validate this live against a dogfood staging warehouse (0000000003b3fbfc, reyden-shadow-read-perf-S, REALTIME, 20.x-snapshot-aarch64-reyden-photon). That warehouse doesn't seem to support native metadata mode, so I couldn't exercise the new native path.

I sent SHOW CATALOGS, SHOW SCHEMAS, SHOW TABLES and SHOW COLUMNS straight to /api/2.0/sql/statements, with and without x-databricks-metadata-operation-type and x-databricks-require-thrift-native-metadata: true. Every response was the regular SHOW-shaped result (catalog, databaseName, col_name/columnType, …), and none of the manifests had is_native_metadata_result. None of the requests errored either, even with the require header set.

I also ran an ad-hoc probe of 25 metadata calls through the driver, on this PR (65bd2ee) and on its base (6dd6b1d). It covered GetCatalogs, GetSchemas, GetTables, GetColumns, GetPrimaryKeys, GetTableSchema and GetObjects with literal and wildcard catalogs. The output was identical on both, so sending the new headers doesn't break the SHOW fallback. But none of the native code (NativeMetadataResultBuilder, NativeMetadataColumns, the native GetObjects/GetColumns branches) ran.

Questions:

  1. Which warehouse, DBR/DBSQL version or server-side flag returns is_native_metadata_result: true? Was the comparator run (36931597557) against one of those?
  2. Is it expected that x-databricks-require-thrift-native-metadata: true falls back silently instead of failing when native mode isn't available? If so, the SHOW-parsing fallback isn't dead code and should stay. A short comment saying that would help.
  3. Could we add an E2E test that asserts the native path is taken, e.g. by checking IsNativeMetadataResult, on a warehouse that supports it? Without one, a server that silently falls back would hide regressions in the native code.

@eric-wang-1990

Copy link
Copy Markdown
Collaborator

Suggestion: decode native and SHOW results once, at the boundary

Right now each consumer decodes both result formats inline. The PR adds about 100 isNative references across StatementExecutionConnection.cs and StatementExecutionStatement.cs. The column readers in GetTableSchema, IGetObjectsDataProvider.PopulateColumnInfoAsync and the statement's GetColumnsAsync are the worst. Each fetches the SHOW arrays as isNative ? null : TryGetColumn(...), then reads every field with isNative ? native!.String(...) : arr!.IsNull(i) ? null : arr.GetString(i). That's hard to follow, and it's how the native-only rules drifted apart between the methods.

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 main. For example, GetTableSchema:

var fields = ReadColumns(resolvedCatalog, dbSchema, tableName, null, cts.Token)
    .Select(c => new Field(c.Name, ToArrowType(c.TypeName), c.Nullable))
    .ToList();

PopulateColumnInfoAsync and the flat GetColumnsAsync become a single loop calling PopulateTableInfoFromTypeName(info, c.Name, c.TypeName, c.Ordinal, c.Nullable, c.Default, c.IsAutoIncrement).

What else this removes or fixes:

  1. The batch-slicing loop in ExecuteNativeShowColumnsAsync goes away. It currently splits each RecordBatch into runs that match the catalog. With rows, that's the one continue filter above. The fan-out can return IEnumerable<ColumnRow> instead of List<(RecordBatch Batch, bool IsNative, string? Catalog)>.
  2. Native-only rules live in one place, each with a comment. That covers the exact-catalog filter, client-side catalog pattern matching for GetCatalogs, the Thrift sort order for GetTables, and the ORDINAL_POSITION convention. Two inconsistencies today come from these rules being spread across methods:
    • PopulateColumnInfoAsync uses ORDINAL_POSITION + 1, but the mixed path in the statement's GetColumnsAsync uses the raw value.
    • On native results, IGetObjectsDataProvider.GetTablesAsync compares the GetObjects catalog pattern to each row's catalog with an exact string.Equals. With catalogPattern = "ma%", GetCatalogsAsync correctly matches main, then every table is dropped, so GetObjects returns catalogs and schemas but no tables. GetSchemasAsync similarly falls back to the raw pattern as the catalog key when TABLE_CATALOG is null. Inside a decoder it's clear that GetObjects needs pattern matching (MatchesCatalogPattern), while flat GetTables wants an exact match.
  3. Naming. ExecuteNativeMetadataSqlAsync may return SHOW-shaped results, so "Native" in the name is misleading. Something like ExecuteMetadataCommandAsync(sql, operation) returning MetadataBatches { Batches, IsNative } reads better than a bare (List<RecordBatch>, bool) tuple.
  4. The mixed native/SHOW fallback in the statement's GetColumnsAsync can go. All fanned-out requests hit the same warehouse, so mixed results shouldn't happen in practice. With ColumnRow it doesn't matter anyway, because both formats land in the same TableInfo path.

One fork remains. For flat results, NativeMetadataResultBuilder.Build passes server columns through, while FlatColumnsResultBuilder builds them from TableInfo (e.g. REMARKS is hardcoded to ""). That can stay as a single early if (result.IsNative) return NativeMetadataResultBuilder.Build(...) at the top of each Get*Async. Most operations already do this. Alternatively, add the missing fields (e.g. Remarks) to ColumnRow/TableInfo so flat GetColumns needs no native branch at all.

@eric-wang-1990

Copy link
Copy Markdown
Collaborator

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: SHOW CATALOGS returns main/other, SHOW SCHEMAS returns main.default, and SHOW TABLES returns main.default.t1. Then I called GetObjects(Tables, catalogPattern, "default", …):

catalogPattern SQL sent Result
main SHOW TABLES IN CATALOG `main` SCHEMA LIKE 'default' main / default / [t1] ✅
null SHOW TABLES IN ALL CATALOGS SCHEMA LIKE 'default' main / default / [t1] ✅
ma% SHOW TABLES IN CATALOG `ma%` SCHEMA LIKE 'default' main / default / [] ❌

The native branch of IGetObjectsDataProvider.GetTablesAsync compares the GetObjects catalog pattern to each row's catalog as an exact string (!string.Equals(catalogPattern, cat, …)). GetCatalogsAsync correctly matches main with MatchesCatalogPattern, but every table is then filtered out. For comparison, the current SHOW path returns 132 rows for the same ma% / default call on a live dogfood warehouse. The fix is probably to use MatchesCatalogPattern(catalogPattern, cat) there. GetSchemasAsync's fallback to catalogPattern when TABLE_CATALOG is null has the same problem. A [Theory] over main / ma% / null would make a good regression test.

2. Correction: the ORDINAL_POSITION handling is fine. In my earlier comment I called the +1 in PopulateColumnInfoAsync versus the raw value in flat GetColumnsAsync an inconsistency. It isn't. The Thrift server sends 0-based positions, and the Thrift path adds PositionRequiredOffset (1 for Spark/Databricks) only for GetObjects. Live, flat GetColumns is 0-based and GetObjects is 1-based on both Thrift and SEA, which matches what this PR does. Please disregard that point.

3. COLUMN_SIZE / DECIMAL_DIGITS defaults match Thrift. I compared 26 columns between SEA and real Thrift (PRO warehouse): BIGINT, INT, DECIMAL(18,2), DOUBLE, DATE, TIMESTAMP and STRING. Both fields are identical, so recomputing them from TYPE_NAME is consistent with Thrift for these types. BUFFER_LENGTH, CHAR_OCTET_LENGTH and SQL_DATETIME_SUB differ (SEA fills them, Thrift returns null), but those differences already exist on main.

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 x-databricks-require-thrift-native-metadata: true and silently return SHOW results, so the questions in my first comment still stand.

@vuanhphung
vuanhphung force-pushed the vu-phung/csharp-native-metadata branch from 65bd2ee to 3d7ea2a Compare October 6, 2026 18:59
@github-actions github-actions Bot removed the integration-test Trigger integration tests in internal repo label Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

🔒 Integration test approval reset

New commits were pushed to this PR. The integration-test label has been automatically removed for security.

A maintainer must re-review the changes and re-add the label to trigger tests again.

Why is this necessary?
  • New code requires fresh security review
  • Prevents approved PRs from adding malicious code later
  • Ensures all tested code has been explicitly approved

Latest commit: 3d7ea2a

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread csharp/src/StatementExecution/StatementExecutionConnection.cs Outdated

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.)

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread csharp/src/StatementExecution/StatementExecutionConnection.cs

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread csharp/src/StatementExecution/StatementExecutionConnection.cs Outdated
Comment thread csharp/src/StatementExecution/StatementExecutionConnection.cs
@vuanhphung vuanhphung added the integration-test Trigger integration tests in internal repo label Oct 7, 2026

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🚀 Integration tests triggered! View workflow run

@vuanhphung vuanhphung added the e2e-test Trigger E2E tests on this PR label Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🚀 Integration tests triggered! View workflow run

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@github-actions github-actions Bot removed the integration-test Trigger integration tests in internal repo label Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🔒 Integration test approval reset

New commits were pushed to this PR. The integration-test label has been automatically removed for security.

A maintainer must re-review the changes and re-add the label to trigger tests again.

Why is this necessary?
  • New code requires fresh security review
  • Prevents approved PRs from adding malicious code later
  • Ensures all tested code has been explicitly approved

Latest commit: dca1b58

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread csharp/src/StatementExecution/StatementExecutionConnection.cs
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🔒 Integration test approval reset

New commits were pushed to this PR. The integration-test label has been automatically removed for security.

A maintainer must re-review the changes and re-add the label to trigger tests again.

Why is this necessary?
  • New code requires fresh security review
  • Prevents approved PRs from adding malicious code later
  • Ensures all tested code has been explicitly approved

Latest commit: 92db613

@vuanhphung vuanhphung added the integration-test Trigger integration tests in internal repo label Oct 9, 2026

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🚀 Integration tests triggered! View workflow run

@vuanhphung vuanhphung added e2e-test Trigger E2E tests on this PR and removed e2e-test Trigger E2E tests on this PR labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🚀 Integration tests triggered! View workflow run

@vuanhphung

Copy link
Copy Markdown
Collaborator Author

@eric-wang-1990

which warehouse/flag returns is_native_metadata_result: true

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>
@github-actions github-actions Bot removed the integration-test Trigger integration tests in internal repo label Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🔒 Integration test approval reset

New commits were pushed to this PR. The integration-test label has been automatically removed for security.

A maintainer must re-review the changes and re-add the label to trigger tests again.

Why is this necessary?
  • New code requires fresh security review
  • Prevents approved PRs from adding malicious code later
  • Ensures all tested code has been explicitly approved

Latest commit: f0ccb1d

@vuanhphung vuanhphung added integration-test Trigger integration tests in internal repo e2e-test Trigger E2E tests on this PR and removed e2e-test Trigger E2E tests on this PR labels Oct 9, 2026

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🚀 Integration tests triggered! View workflow run

2 similar comments
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🚀 Integration tests triggered! View workflow run

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

🚀 Integration tests triggered! View workflow run

@vuanhphung

Copy link
Copy Markdown
Collaborator Author

@eric-wang-1990 confirm latest E2E tests are passing with comparator runs not showing any new diff https://databricks.slack.com/archives/C0B70MDS8DP/p1791560007960709.

@eric-wang-1990

Copy link
Copy Markdown
Collaborator

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 f0ccb1d. One structural follow-up I'd like to propose, not for this PR.

MetadataRowReader.cs is now ~600 lines with 10 types and four concerns: catalog filtering, command results, row types, and decoding, where Catalogs/Schemas/Tables still branch on result.IsNative per field. Meanwhile the legacy (SHOW) flat results are still built inline in each StatementExecutionStatement.Get*Async, while native flat results go through NativeMetadataResultBuilder. Mirroring the two formats would make the difference explicit, let the compiler keep them in sync, and make the legacy path easy to delete once all warehouses return native metadata (19.x+ per your note).

Proposed layout (folders map to namespaces, like MetadataCommands/):

StatementExecution/Metadata/
  IMetadataDecoder.cs        contract: Catalogs / Schemas / Tables / Columns → rows
  IMetadataResultBuilder.cs  contract: flat Thrift-shaped QueryResult per operation
  MetadataRowReader.cs       picks Native vs Legacy once (result.IsNative) + rules shared by both formats
                             (table-type filter, DefaultTableType, GetObjects ordinal offset)
  MetadataRows.cs            SchemaRow / TableRow / ColumnRow / PrimaryKeyRow / ForeignKeyRow
  MetadataBatches.cs         MetadataBatches + ColumnMetadataResult
  CatalogFilter.cs           CatalogFilter + pattern matching (build the regex once per filter)
  Native/
    Catalogs.cs  Schemas.cs  Tables.cs  Columns.cs   ← same file names as Legacy/
    ResultBuilder.cs                                 ← today's NativeMetadataResultBuilder
    NativeMetadataColumns.cs (+ NativeMetadataRow)   ← native-only helper
  Legacy/
    Catalogs.cs  Schemas.cs  Tables.cs  Columns.cs
    ResultBuilder.cs                                 ← the StringArray.Builder blocks now inline in Get*Async
    PrimaryKeys.cs  ForeignKeys.cs                   ← SHOW-only today (native PK/FK go straight to the builder)

Each folder's class is a partial across the per-operation files and implements the interfaces, so an operation added on one side only fails the build:

// 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:

  • Native/ owns the native-only compensation: CatalogFilter on rows (native expands a quoted catalog as LIKE), the Thrift table sort, zero-based ORDINAL_POSITION, Thrift null defaults, server-column passthrough in the flat result.
  • Legacy/ owns SHOW column names and parsing. Keep its current behavior exactly (e.g. no client-side CatalogFilter, normalizeEmptyTableType), in line with 091c585.
  • Metadata/ owns only rules that apply to both formats, so they can't drift apart again (the root cause of the earlier ma% bug).

With the result builders mirrored, the statement's Get*Async methods stop branching on IsNative entirely. They run the command, pick the builder, and return, which also takes most of the format-specific code out of the 2,000-line StatementExecutionStatement.cs.

Tests: one parity [Theory] per operation that runs both decoders on equivalent fixtures (the same logical metadata in native shape and in SHOW shape) and asserts identical rows, plus a few format-specific tests (Thrift sort, native LIKE expansion). Most of MetadataRowReaderTests splits along that line.

Happy to help with or review the follow-up.

@vuanhphung

Copy link
Copy Markdown
Collaborator Author

@eric-wang-1990 sure I'll follow up on that after this PR. Is this fine to merge now?

@vuanhphung
vuanhphung added this pull request to the merge queue Oct 12, 2026
Merged via the queue into main with commit 2db6d31 Oct 12, 2026
65 of 69 checks passed
@vuanhphung
vuanhphung deleted the vu-phung/csharp-native-metadata branch October 12, 2026 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-assisted e2e-test Trigger E2E tests on this PR integration-test Trigger integration tests in internal repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants