Skip to content

Commit d6ee0bc

Browse files
fix(csharp): address issue #544 (1 review thread)
Addresses: - #3450669941 at csharp/src/DatabricksStatement.cs:951
1 parent 7ca6e23 commit d6ee0bc

1 file changed

Lines changed: 29 additions & 9 deletions

File tree

csharp/src/DatabricksStatement.cs

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -906,6 +906,10 @@ private sealed class NormalizingTablesStream : IArrowArrayStream
906906
private readonly int _tableTypeIndex;
907907
private readonly int _remarksIndex;
908908

909+
// Set once if a column we intended to normalize arrives as a non-StringArray layout, so
910+
// the Release-safe warning below is emitted a single time per stream rather than per batch.
911+
private bool _warnedUnexpectedColumnType;
912+
909913
public NormalizingTablesStream(IArrowArrayStream inner, int tableTypeIndex, int remarksIndex)
910914
{
911915
_inner = inner ?? throw new ArgumentNullException(nameof(inner));
@@ -931,10 +935,11 @@ private RecordBatch NormalizeBatch(RecordBatch batch)
931935
// ASSUMPTION: the metadata schema declares TABLE_TYPE/REMARKS as StringType.Default,
932936
// so they arrive as StringArray. If a future schema change makes either column a
933937
// different string layout (e.g. LargeStringArray), the `is StringArray` checks below
934-
// fail and the column falls through to the unnormalized `else` branch — silently
935-
// reopening issue #527. The Debug.Assert below makes that mismatch observable in
936-
// debug builds instead of failing silently; add a LargeStringArray branch (or
937-
// generalize NormalizeStringColumn) if the schema ever changes.
938+
// fail and the column falls through to the unnormalized `else` branch, reopening
939+
// issue #527. The `else` branch makes that mismatch observable in BOTH builds (a
940+
// Debug.Assert plus a Release-safe Trace.TraceWarning) instead of failing silently;
941+
// add a LargeStringArray branch (or generalize NormalizeStringColumn) if the schema
942+
// ever changes.
938943
if (i == _tableTypeIndex && batch.Column(i) is StringArray tableTypeArray)
939944
{
940945
columns[i] = NormalizeStringColumn(tableTypeArray, DefaultTableType, normalizeUnknown: false);
@@ -952,11 +957,26 @@ private RecordBatch NormalizeBatch(RecordBatch batch)
952957
{
953958
// Guardrail: if this is a column we intended to normalize but its array type is
954959
// not StringArray, the type guards above fell through and #527 is silently
955-
// reopened. Surface that as an assertion failure in debug builds rather than
956-
// leaving it invisible at runtime.
957-
Debug.Assert(
958-
i != _tableTypeIndex && i != _remarksIndex,
959-
$"Expected TABLE_TYPE/REMARKS column at index {i} to be a StringArray but got {batch.Column(i).GetType().Name}; normalization (issue #527) was skipped.");
960+
// reopened. The metadata schema layout is server-controlled and could change
961+
// without a code change here, so signal in BOTH builds: a Debug.Assert that
962+
// fails loudly in debug, and a Trace.TraceWarning (compiled in under the TRACE
963+
// constant) that remains observable in Release. The warning is emitted at most
964+
// once per stream to avoid per-batch log spam.
965+
if (i == _tableTypeIndex || i == _remarksIndex)
966+
{
967+
string columnName = i == _tableTypeIndex ? "TABLE_TYPE" : "REMARKS";
968+
string actualType = batch.Column(i).GetType().Name;
969+
Debug.Assert(
970+
false,
971+
$"Expected {columnName} column at index {i} to be a StringArray but got {actualType}; normalization (issue #527) was skipped.");
972+
if (!_warnedUnexpectedColumnType)
973+
{
974+
_warnedUnexpectedColumnType = true;
975+
Trace.TraceWarning(
976+
$"GetTables metadata column {columnName} at index {i} is {actualType}, not StringArray; " +
977+
"TABLE_TYPE/REMARKS normalization (issue #527) was skipped. Add a branch for this array type if the schema changed.");
978+
}
979+
}
960980
columns[i] = batch.Column(i);
961981
}
962982
}

0 commit comments

Comments
 (0)