Skip to content

Commit fd35ac8

Browse files
msrathore-dbclaude
andcommitted
Clean up code review issues
- Remove accidentally committed databricks-jdbc copy submodule - Clean up protocol-specific comments (replace Thrift/JDBC references with generic descriptions) - Use static methods in ColumnTypeMapper for stateless operations - Update SparkConnection to use static ColumnTypeMapper methods - Update XML doc comments in metadata record classes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent add4656 commit fd35ac8

12 files changed

+144
-80
lines changed

csharp/databricks-jdbc copy

Lines changed: 0 additions & 1 deletion
This file was deleted.

csharp/src/Drivers/Apache/Hive2/HiveServer2Connection.cs

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -108,13 +108,14 @@ internal struct ColumnsMetadataColumnNames
108108
}
109109

110110
/// <summary>
111-
/// The data type definitions based on the <see href="https://docs.oracle.com/en%2Fjava%2Fjavase%2F21%2Fdocs%2Fapi%2F%2F/java.sql/java/sql/Types.html">JDBC Types</see> constants.
111+
/// The data type definitions based on SQL/CLI specification (ISO/IEC 9075-3).
112112
/// </summary>
113113
/// <remarks>
114-
/// This enumeration can be used to determine the drivers specific data types that are contained in fields <c>xdbc_data_type</c> and <c>xdbc_sql_data_type</c>
114+
/// This enumeration can be used to determine the driver's specific data types that are contained in fields <c>xdbc_data_type</c> and <c>xdbc_sql_data_type</c>
115115
/// in the column metadata <see cref="StandardSchemas.ColumnSchema"/>. This column metadata is returned as a result of a call to
116116
/// <see cref="AdbcConnection.GetObjects(GetObjectsDepth, string?, string?, string?, IReadOnlyList{string}?, string?)"/>
117117
/// when <c>depth</c> is set to <see cref="AdbcConnection.GetObjectsDepth.All"/>.
118+
/// For new code, prefer using <see cref="Metadata.ColumnTypeId"/> instead.
118119
/// </remarks>
119120
internal enum ColumnTypeId
120121
{
@@ -652,7 +653,7 @@ public override IArrowArrayStream GetObjects(GetObjectsDepth depth, string? cata
652653
isNullable.Equals("NO", StringComparison.InvariantCultureIgnoreCase) ? false :
653654
(bool?)null;
654655

655-
// Get precision/scale from Thrift using SetPrecisionScaleAndTypeName
656+
// Extract precision/scale using protocol-specific parsing
656657
var tempTableInfo = new TableInfo(string.Empty);
657658
SetPrecisionScaleAndTypeName(colType, typeName, tempTableInfo, columnSize, decimalDigits);
658659

@@ -665,22 +666,22 @@ public override IArrowArrayStream GetObjects(GetObjectsDepth depth, string? cata
665666
typeName,
666667
ordinalPos,
667668
isNullableBool,
668-
remarks: null, // Not available in Thrift GetObjects
669+
remarks: null,
669670
columnDefault: columnDefault,
670671
customData: null
671672
);
672673

673-
// Override ALL fields with Thrift-provided values to preserve exact original behavior
674-
record.XdbcDataType = colType; // From Thrift
675-
record.XdbcColumnSize = tempTableInfo.Precision.Count > 0 ? tempTableInfo.Precision[0] : null; // From Thrift/parsing
676-
record.XdbcDecimalDigits = tempTableInfo.Scale.Count > 0 ? tempTableInfo.Scale[0] : null; // From Thrift/parsing
677-
record.XdbcNumPrecRadix = null; // Original Thrift: always null
678-
record.SqlDataType = colType; // From Thrift (same as XdbcDataType)
679-
record.XdbcCharOctetLength = null; // Original Thrift: always null
680-
record.SqlDatetimeSub = null; // Original Thrift: always null
681-
record.Nullable = nullable; // From Thrift
682-
record.IsNullable = isNullable; // From Thrift
683-
record.IsAutoIncrement = isAutoIncrement ? "YES" : "NO"; // From Thrift
674+
// Override synthesized fields with protocol-provided values
675+
record.XdbcDataType = colType;
676+
record.XdbcColumnSize = tempTableInfo.Precision.Count > 0 ? tempTableInfo.Precision[0] : null;
677+
record.XdbcDecimalDigits = tempTableInfo.Scale.Count > 0 ? tempTableInfo.Scale[0] : null;
678+
record.XdbcNumPrecRadix = null;
679+
record.SqlDataType = colType;
680+
record.XdbcCharOctetLength = null;
681+
record.SqlDatetimeSub = null;
682+
record.Nullable = nullable;
683+
record.IsNullable = isNullable;
684+
record.IsAutoIncrement = isAutoIncrement ? "YES" : "NO";
684685

685686
tableMeta.Value.Columns.Add(record);
686687
}
@@ -1487,7 +1488,7 @@ private static IArrowType GetArrowType(int columnTypeId, string typeName, bool i
14871488
else
14881489
{
14891490
// Note: parsing the type name for SQL DECIMAL types as the precision and scale values
1490-
// may not be returned in the Thrift call to GetColumns
1491+
// may not be returned in the GetColumns response
14911492
return SqlTypeNameParser<SqlDecimalParserResult>
14921493
.Parse(typeName, columnTypeId)
14931494
.Decimal128Type;

csharp/src/Drivers/Apache/Hive2/HiveServer2Statement.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ protected virtual void SetStatementProperties(TExecuteStatementReq statement)
7070
}
7171

7272
/// <summary>
73-
/// Gets the schema from metadata response. Base implementation uses traditional Thrift schema.
74-
/// Subclasses can override to support Arrow schema parsing.
73+
/// Gets the schema from metadata response. Base implementation uses the standard schema parser.
74+
/// Subclasses can override to support alternative schema parsing strategies.
7575
/// </summary>
7676
/// <param name="metadata">The metadata response containing schema information</param>
7777
/// <returns>The Arrow schema</returns>
@@ -624,7 +624,7 @@ protected internal QueryResult EnhanceGetColumnsResult(Schema originalSchema, IR
624624
customData: null
625625
);
626626

627-
// Extract values with fallback to Thrift-provided values
627+
// Extract values with fallback to protocol-provided values
628628
string baseTypeName = record.BaseTypeName ?? typeName ?? string.Empty;
629629
int finalColumnSize = record.XdbcColumnSize ?? columnSize;
630630
int finalDecimalDigits = record.XdbcDecimalDigits ?? decimalDigits;

csharp/src/Drivers/Apache/Hive2/Metadata/CatalogMetadataRecord.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ namespace Apache.Arrow.Adbc.Drivers.Apache.Hive2.Metadata
1919
{
2020
/// <summary>
2121
/// Protocol-agnostic data model for catalog metadata.
22-
/// Used by both HiveServer2 (Thrift) and StatementExecution API (REST) protocols.
22+
/// Supports multiple protocol implementations including REST and RPC-based protocols.
2323
/// </summary>
2424
public class CatalogMetadataRecord
2525
{

csharp/src/Drivers/Apache/Hive2/Metadata/ColumnMetadataRecord.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ namespace Apache.Arrow.Adbc.Drivers.Apache.Hive2.Metadata
2121
{
2222
/// <summary>
2323
/// Protocol-agnostic data model for column metadata.
24-
/// Used by both HiveServer2 (Thrift) and StatementExecution API (REST) protocols.
25-
/// Contains 24 fields: 23 standard fields plus BASE_TYPE_NAME (a Databricks/Spark extension).
24+
/// Supports multiple protocol implementations including REST and RPC-based protocols.
25+
/// Contains 24 fields: 23 standard XDBC fields plus BASE_TYPE_NAME (a Databricks/Spark extension).
2626
/// </summary>
2727
public class ColumnMetadataRecord
2828
{
@@ -70,8 +70,8 @@ public class ColumnMetadataRecord
7070

7171
/// <summary>
7272
/// Transfer size of the data in bytes (BUFFER_LENGTH in statement-based metadata)
73-
/// Nullable for most types, only applicable for certain binary types
74-
/// Uses sbyte to match Int8Type from original Thrift schema
73+
/// Nullable for most types, only applicable for certain binary types.
74+
/// Uses sbyte to match Int8Type as per the ADBC schema specification.
7575
/// </summary>
7676
public sbyte? BufferLength { get; set; }
7777

0 commit comments

Comments
 (0)