fix(csharp): metadata(csharp): GetTables TABLE_TYPE/REMARKS/TABLE_NAME value normalization differs (Thrift REMARKS=UNKNOWN vs SEA "") (#527)

csharp/src/DatabricksStatement.cs

@@ -37,6 +37,7 @@
 using OperationType = AdbcDrivers.Databricks.Telemetry.Proto.Operation.Types.Type;
 using Apache.Arrow;
 using Apache.Arrow.Adbc;
+using Apache.Arrow.Ipc;
 using AdbcDrivers.HiveServer2;
 using AdbcDrivers.HiveServer2.Hive2;
 using AdbcDrivers.HiveServer2.Spark;
@@ -814,12 +815,176 @@ protected override async Task<QueryResult> GetTablesAsync(CancellationToken canc
                 // Call the base implementation with the potentially modified catalog name
                 activity?.AddEvent("statement.get_tables.calling_base_implementation");
                 QueryResult result = await base.GetTablesAsync(cancellationToken);
+
+                // Normalize TABLE_TYPE/REMARKS so the Thrift path matches SEA (issue #527).
+                // The legacy hive_metastore Thrift response returns REMARKS="UNKNOWN" and an empty
+                // TABLE_TYPE for tables with no comment. SEA returns REMARKS="" and TABLE_TYPE="TABLE";
+                // the empty-TABLE_TYPE -> "TABLE" default also matches the official JDBC driver's
+                // MetadataResultSetBuilder (the REMARKS="" parity is with SEA, not a JDBC rule).
+                // Scope note (issue #527): only TABLE_TYPE/REMARKS carry placeholder values that
+                // diverge between Thrift and SEA. TABLE_NAME is the server-supplied table identifier
+                // and is returned identically by both paths, so it is intentionally not normalized
+                // here (rewriting it would risk corrupting a real name).
+                result = NormalizeTablesResult(result);
+
                 activity?.SetTag(SemanticConventions.Db.Response.ReturnedRows, result.RowCount);
                 activity?.AddEvent("statement.get_tables.complete");
                 return result;
             }, activityName: "GetTables");
         }
 
+        // Canonical TABLE_TYPE used when the server returns a null/empty classification,
+        // matching the JDBC driver's MetadataResultSetBuilder default.
+        private const string DefaultTableType = "TABLE";
+
+        // Exact sentinel some legacy Thrift servers (e.g. hive_metastore) emit for REMARKS
+        // when a table has no comment; SEA returns "" instead. Matched case-sensitively so a
+        // genuine user comment of "Unknown"/"unknown" is preserved rather than erased.
+        private const string UnknownRemarksPlaceholder = "UNKNOWN";
+
+        /// <summary>
+        /// Wraps a GetTables result so the TABLE_TYPE and REMARKS columns are normalized to match
+        /// SEA/JDBC values (issue #527):
+        ///   - TABLE_TYPE: null/empty defaults to "TABLE".
+        ///   - REMARKS: null/empty or the legacy "UNKNOWN" placeholder defaults to "".
+        /// The normalization is applied lazily, one batch at a time, via
+        /// <see cref="NormalizingTablesStream"/> so a large (and potentially CloudFetch-backed)
+        /// result is not buffered into memory. Returns the original result unchanged when there
+        /// is nothing to normalize. Row count is preserved because normalization only rewrites
+        /// values in place and never adds or drops rows.
+        /// </summary>
+        private static QueryResult NormalizeTablesResult(QueryResult result)
+        {
+            if (result.Stream == null)
+            {
+                return result;
+            }
+
+            Schema schema = result.Stream.Schema;
+            int tableTypeIndex = schema.GetFieldIndex("TABLE_TYPE");
+            int remarksIndex = schema.GetFieldIndex("REMARKS");
+
+            // Nothing to normalize if neither column is present.
+            if (tableTypeIndex < 0 && remarksIndex < 0)
+            {
+                return result;
+            }
+
+            return new QueryResult(result.RowCount, new NormalizingTablesStream(result.Stream, tableTypeIndex, remarksIndex));
+        }
+
+        /// <summary>
+        /// Builds a normalized copy of a string column. Null/empty values are replaced with
+        /// <paramref name="defaultValue"/>; when <paramref name="normalizeUnknown"/> is true the
+        /// exact (case-sensitive) legacy "UNKNOWN" sentinel is also replaced with
+        /// <paramref name="defaultValue"/>, leaving genuine comments like "Unknown" untouched.
+        /// </summary>
+        private static StringArray NormalizeStringColumn(StringArray source, string defaultValue, bool normalizeUnknown)
+        {
+            var builder = new StringArray.Builder();
+            for (int i = 0; i < source.Length; i++)
+            {
+                string value = source.IsNull(i) ? string.Empty : source.GetString(i);
+                if (string.IsNullOrEmpty(value)
+                    || (normalizeUnknown && string.Equals(value, UnknownRemarksPlaceholder, StringComparison.Ordinal)))
+                {
+                    value = defaultValue;
+                }
+                builder.Append(value);
+            }
+            return builder.Build();
+        }
+
+        /// <summary>
+        /// Lazy <see cref="IArrowArrayStream"/> wrapper that normalizes the TABLE_TYPE and REMARKS
+        /// columns of each batch as it is read from the underlying stream, preserving the
+        /// incremental/streaming consumption of the wrapped result instead of buffering it.
+        /// </summary>
+        private sealed class NormalizingTablesStream : IArrowArrayStream
+        {
+            private readonly IArrowArrayStream _inner;
+            private readonly int _tableTypeIndex;
+            private readonly int _remarksIndex;
+
+            // Set once if a column we intended to normalize arrives as a non-StringArray layout, so
+            // the Release-safe warning below is emitted a single time per stream rather than per batch.
+            private bool _warnedUnexpectedColumnType;
+
+            public NormalizingTablesStream(IArrowArrayStream inner, int tableTypeIndex, int remarksIndex)
+            {
+                _inner = inner ?? throw new ArgumentNullException(nameof(inner));
+                _tableTypeIndex = tableTypeIndex;
+                _remarksIndex = remarksIndex;
+            }
+
+            public Schema Schema => _inner.Schema;
+
+            public async ValueTask<RecordBatch?> ReadNextRecordBatchAsync(CancellationToken cancellationToken = default)
+            {
+                RecordBatch? batch = await _inner.ReadNextRecordBatchAsync(cancellationToken).ConfigureAwait(false);
+                return batch == null ? null : NormalizeBatch(batch);
+            }
+
+            public void Dispose() => _inner.Dispose();
+
+            private RecordBatch NormalizeBatch(RecordBatch batch)
+            {
+                var columns = new IArrowArray[batch.ColumnCount];
+                for (int i = 0; i < batch.ColumnCount; i++)
+                {
+                    // ASSUMPTION: the metadata schema declares TABLE_TYPE/REMARKS as StringType.Default,
+                    // so they arrive as StringArray. If a future schema change makes either column a
+                    // different string layout (e.g. LargeStringArray), the `is StringArray` checks below
+                    // fail and the column falls through to the unnormalized `else` branch, reopening
+                    // issue #527. The `else` branch makes that mismatch observable in BOTH builds (a
+                    // Debug.Assert plus a Release-safe Trace.TraceWarning) instead of failing silently;
+                    // add a LargeStringArray branch (or generalize NormalizeStringColumn) if the schema
+                    // ever changes.
+                    if (i == _tableTypeIndex && batch.Column(i) is StringArray tableTypeArray)
+                    {
+                        columns[i] = NormalizeStringColumn(tableTypeArray, DefaultTableType, normalizeUnknown: false);
+                        // The source array is replaced and no longer referenced by the new batch; dispose
+                        // it now rather than leaving its Arrow buffers to the finalizer. Untouched columns
+                        // are reused by reference, so they must NOT be disposed here.
+                        tableTypeArray.Dispose();
+                    }
+                    else if (i == _remarksIndex && batch.Column(i) is StringArray remarksArray)
+                    {
+                        columns[i] = NormalizeStringColumn(remarksArray, string.Empty, normalizeUnknown: true);
+                        remarksArray.Dispose();
+                    }
+                    else
+                    {
+                        // Guardrail: if this is a column we intended to normalize but its array type is
+                        // not StringArray, the type guards above fell through and #527 is silently
+                        // reopened. The metadata schema layout is server-controlled and could change
+                        // without a code change here, so signal in BOTH builds: a Debug.Assert that
+                        // fails loudly in debug, and a Trace.TraceWarning (compiled in under the TRACE
+                        // constant) that remains observable in Release. The warning is emitted at most
+                        // once per stream to avoid per-batch log spam.
+                        if (i == _tableTypeIndex || i == _remarksIndex)
+                        {
+                            string columnName = i == _tableTypeIndex ? "TABLE_TYPE" : "REMARKS";
+                            string actualType = batch.Column(i).GetType().Name;
+                            Debug.Assert(
+                                false,
+                                $"Expected {columnName} column at index {i} to be a StringArray but got {actualType}; normalization (issue #527) was skipped.");
+                            if (!_warnedUnexpectedColumnType)
+                            {
+                                _warnedUnexpectedColumnType = true;
+                                Trace.TraceWarning(
+                                    $"GetTables metadata column {columnName} at index {i} is {actualType}, not StringArray; " +
+                                    "TABLE_TYPE/REMARKS normalization (issue #527) was skipped. Add a branch for this array type if the schema changed.");
+                            }
+                        }
+                        columns[i] = batch.Column(i);
+                    }
+                }
+
+                return new RecordBatch(Schema, columns, batch.Length);
+            }
+        }
+
         /// <summary>
         /// Overrides the GetColumnsAsync method to handle the SPARK catalog case.
         /// When EnableMultipleCatalogSupport is true:

csharp/test/E2E/StatementExecution/SeaMetadataE2ETests.cs

@@ -163,6 +163,86 @@ public async Task GetTables_ReturnsAllColumnTypes()
             Assert.True(row.ContainsKey("REF_GENERATION"), "Should have REF_GENERATION column");
         }
 
+        // Regression test for #527 (Thrift vs SEA GetTables value parity).
+        //
+        // The legacy hive_metastore catalog's Thrift TGetTablesResp returns
+        // placeholder values that SEA/JDBC do not:
+        //   - REMARKS    = "UNKNOWN"  (should normalize to "")
+        //   - TABLE_TYPE = ""         (should normalize to "TABLE")
+        // The Databricks Thrift GetTables path must normalize these so the
+        // values match SEA. hive_metastore is a built-in catalog on most
+        // Databricks workspaces, so no fixture setup is required; the test
+        // skips (rather than fails) where legacy access is disabled or the
+        // default schema is empty.
+        //
+        // AUTHORITATIVE REGRESSION GUARD: the unit tests
+        // NormalizeStringColumn_Remarks_NormalizesNullEmptyAndUnknownCaseSensitively
+        // and NormalizeStringColumn_TableType_DefaultsNullEmptyButLeavesUnknown
+        // (DatabricksStatementUnitTests) are the real red→green coverage: they
+        // feed known placeholder inputs and assert the rewritten output. This
+        // E2E case only asserts negative invariants against live workspace
+        // state, so if hive_metastore.default happens to contain only tables
+        // that never carried the placeholders, it can pass green even with the
+        // fix reverted. Treat it as a live-parity smoke check, not the guard.
+        //
+        // NOTE: This test deliberately overrides the run-selected protocol and
+        // pins Protocol = "thrift", which is an intentional exception to the
+        // class-level convention documented above CreateConnection ("Tests
+        // never pick the protocol themselves"). The placeholder values being
+        // normalized (REMARKS = "UNKNOWN", TABLE_TYPE = "") only ever appear on
+        // the legacy Thrift hive_metastore path; SEA never returns them, so a
+        // protocol-agnostic assertion would silently pass on the SEA/Reyden
+        // nightly run without exercising the fix. The pin is required to cover
+        // the regression — please don't "fix" it back to the convention.
+        [SkippableFact]
+        public async Task GetTables_NormalizesRemarksAndTableType_Thrift()
+        {
+            SkipIfNotConfigured();
+            using var conn = CreateConnection(new Dictionary<string, string>
+            {
+                { DatabricksParameters.Protocol, "thrift" },
+                // hive_metastore is a non-default catalog; multi-catalog support
+                // must be enabled for GetTables to query it.
+                { DatabricksParameters.EnableMultipleCatalogSupport, "true" }
+            });
+            var rows = await ReadMetadata(conn, "GetTables", "hive_metastore", "default");
+            // hive_metastore is present on most Databricks workspaces, but
+            // Unity-Catalog-only / governance-locked workspaces can have legacy
+            // access disabled, and the `default` schema can legitimately be
+            // empty. In those cases there is nothing to normalize, so skip
+            // rather than hard-fail — coupling this value-parity check to
+            // mutable live workspace state would produce failures unrelated to
+            // the fix. When rows are present the normalization invariants below
+            // still exercise the regression.
+            Skip.If(rows.Count == 0, "hive_metastore.default exposes no tables on this workspace; nothing to normalize");
+
+            // REMARKS must default to "" (matching SEA/JDBC), never the legacy
+            // Thrift placeholder "UNKNOWN". We don't assert every row is "",
+            // because a table may carry a real comment; instead we assert the
+            // normalization invariant. GetStringValue maps a null Arrow value to
+            // the sentinel string "null", so also guard against that to catch a
+            // regression that leaves REMARKS un-normalized (null) rather than "".
+            Assert.DoesNotContain(rows, r => r["REMARKS"] == "UNKNOWN");
+            Assert.DoesNotContain(rows, r => r["REMARKS"] == "null");
+
+            // TABLE_TYPE must always be a non-empty classification. The server
+            // returns "" for hive_metastore managed tables; it must be
+            // normalized to "TABLE". We don't assert that a "TABLE" row is
+            // present, because hive_metastore.default holds live, mutable state
+            // and could legitimately contain only views at test time. We also
+            // don't pin TABLE_TYPE to an exact allow-list: hive_metastore is
+            // live state and may legitimately surface other classifications
+            // (e.g. EXTERNAL/MATERIALIZED-style types), which would make an
+            // exact-membership check flaky for reasons unrelated to this fix.
+            // The invariant under test is purely that normalization fired: the
+            // value is never empty and never the GetStringValue null sentinel.
+            Assert.All(rows, r =>
+            {
+                Assert.False(string.IsNullOrEmpty(r["TABLE_TYPE"]), "TABLE_TYPE must be normalized to a non-empty value");
+                Assert.NotEqual("null", r["TABLE_TYPE"]);
+            });
+        }
+
         // --- GetColumnsExtended ---
 
         [SkippableFact]

csharp/test/Unit/DatabricksStatementUnitTests.cs

@@ -19,6 +19,7 @@
 using System.Reflection;
 using AdbcDrivers.HiveServer2.Spark;
 using AdbcDrivers.Databricks;
+using Apache.Arrow;
 using Xunit;
 using OperationType = AdbcDrivers.Databricks.Telemetry.Proto.Operation.Types.Type;
 
@@ -170,5 +171,76 @@ public void GetMetadataOperationType_IsCaseInsensitive(string command)
         {
             Assert.NotNull(DatabricksStatement.GetMetadataOperationType(command));
         }
+
+        /// <summary>
+        /// Invokes the private static <c>NormalizeStringColumn</c> via reflection so the pure
+        /// GetTables value-parity logic (issue #527) can be unit-tested without a live warehouse.
+        /// </summary>
+        private static StringArray InvokeNormalizeStringColumn(StringArray source, string defaultValue, bool normalizeUnknown)
+        {
+            var method = typeof(DatabricksStatement).GetMethod("NormalizeStringColumn",
+                BindingFlags.NonPublic | BindingFlags.Static);
+            Assert.NotNull(method);
+            return (StringArray)method!.Invoke(null, new object[] { source, defaultValue, normalizeUnknown })!;
+        }
+
+        private static StringArray BuildStringArray(params string?[] values)
+        {
+            var builder = new StringArray.Builder();
+            foreach (string? value in values)
+            {
+                if (value == null)
+                {
+                    builder.AppendNull();
+                }
+                else
+                {
+                    builder.Append(value);
+                }
+            }
+            return builder.Build();
+        }
+
+        private static string?[] ToStrings(StringArray array)
+        {
+            var result = new string?[array.Length];
+            for (int i = 0; i < array.Length; i++)
+            {
+                result[i] = array.IsNull(i) ? null : array.GetString(i);
+            }
+            return result;
+        }
+
+        /// <summary>
+        /// REMARKS normalization (issue #527): null/empty and the legacy case-sensitive "UNKNOWN"
+        /// placeholder default to "", while a genuine "Unknown"/"unknown" comment is preserved.
+        /// </summary>
+        [Fact]
+        public void NormalizeStringColumn_Remarks_NormalizesNullEmptyAndUnknownCaseSensitively()
+        {
+            using var source = BuildStringArray(null, "", "UNKNOWN", "Unknown", "unknown", "a real comment");
+
+            using var result = InvokeNormalizeStringColumn(source, string.Empty, normalizeUnknown: true);
+
+            Assert.Equal(
+                new string?[] { "", "", "", "Unknown", "unknown", "a real comment" },
+                ToStrings(result));
+        }
+
+        /// <summary>
+        /// TABLE_TYPE normalization (issue #527): null/empty default to "TABLE", but the "UNKNOWN"
+        /// sentinel is NOT special-cased here (normalizeUnknown: false), matching the production call.
+        /// </summary>
+        [Fact]
+        public void NormalizeStringColumn_TableType_DefaultsNullEmptyButLeavesUnknown()
+        {
+            using var source = BuildStringArray(null, "", "TABLE", "VIEW", "UNKNOWN");
+
+            using var result = InvokeNormalizeStringColumn(source, "TABLE", normalizeUnknown: false);
+
+            Assert.Equal(
+                new string?[] { "TABLE", "TABLE", "TABLE", "VIEW", "UNKNOWN" },
+                ToStrings(result));
+        }
     }
 }