Copilot commented on code in PR #4919:
URL: https://github.com/apache/arrow-adbc/pull/4919#discussion_r4307778240
##########
csharp/src/Client/SchemaConverter.cs:
##########
@@ -31,13 +31,13 @@ internal class SchemaConverter
/// Converts an Arrow <see cref="Schema"/> to a
/// <see cref="DataTable"/> schema.
/// </summary>
- /// <param name="schema">The Arrow schema</param>
+ /// <param name="fields">The fields of the Arrow schema</param>
/// <param name="adbcStatement">The AdbcStatement to use</param>
/// <exception cref="ArgumentNullException"></exception>
- public static DataTable ConvertArrowSchema(Schema schema,
AdbcStatement adbcStatement, DecimalBehavior decimalBehavior, StructBehavior
structBehavior)
+ public static DataTable ConvertArrowSchema(IReadOnlyList<Field>
fields, AdbcStatement adbcStatement, DecimalBehavior decimalBehavior,
StructBehavior structBehavior)
Review Comment:
This signature introduces `IReadOnlyList<Field>` but the file does not
import `System.Collections.Generic` (and the repository disables implicit
usings in `csharp/Directory.Build.props:43`). The client project therefore
fails to compile with `IReadOnlyList<>` unresolved. Add the using or fully
qualify the type.
##########
csharp/src/Apache.Arrow.Adbc/Extensions/IArrowArrayExtensions.cs:
##########
@@ -156,16 +157,31 @@ public static class IArrowArrayExtensions
}
case ArrowTypeId.Binary:
return ((BinaryArray)arrowArray).GetBytes(index).ToArray();
+ case ArrowTypeId.FixedSizedBinary:
+ return
((FixedSizeBinaryArray)arrowArray).GetBytes(index).ToArray();
case ArrowTypeId.List:
- return ((ListArray)arrowArray).GetSlicedValues(index);
+ return GetListValues((ListArray)arrowArray, index);
case ArrowTypeId.Struct:
StructArray structArray = (StructArray)arrowArray;
return resultType == StructResultType.JsonString ?
SerializeToJson(structArray, index) : ParseStructArray(structArray, index);
+ case ArrowTypeId.Extension:
+ switch (arrowArray)
+ {
+ case GuidArray guidArray:
+ return guidArray.GetGuid(index);
+ case Bool8Array bool8Array:
+ return bool8Array.GetValue(index);
+ case TimestampWithOffsetArray timestampWithOffsetArray:
+ return timestampWithOffsetArray.GetValue(index);
+ case ExtensionArray extensionArray:
+ // Unrecognized extension types are surfaced as
their storage values.
+ return ValueAt(extensionArray.Storage, index,
resultType);
Review Comment:
This fallback is also used when ParseStructArray walks a struct containing a
resolved arrow.parquet.variant child. Unlike a top-level variant, that child is
reduced to its storage array here, so StructBehavior.JsonString/Strict returns
the variant's storage representation instead of VariantJsonWriter output or the
logical Dictionary/List/scalar value. Handle VariantArray in the recursive
conversion path as well.
##########
csharp/src/Client/AdbcCommand.cs:
##########
@@ -298,204 +298,231 @@ private void BindParameters()
for (int i = 0; i < fields.Length; i++)
{
AdbcParameter param =
(AdbcParameter)_dbParameterCollection[i];
+ object? value = param.Value is DBNull ? null : param.Value;
switch (param.DbType)
{
case DbType.Binary:
var binaryBuilder = new BinaryArray.Builder();
- switch (param.Value)
+ switch (value)
{
case null: binaryBuilder.AppendNull(); break;
case byte[] array:
binaryBuilder.Append(array.AsSpan()); break;
- default: throw new
NotSupportedException($"Values of type {param.Value.GetType().Name} cannot be
bound as binary");
+ default: throw new
NotSupportedException($"Values of type {value.GetType().Name} cannot be bound
as binary");
}
parameters[i] = binaryBuilder.Build();
break;
case DbType.Boolean:
var boolBuilder = new BooleanArray.Builder();
- switch (param.Value)
+ switch (value)
{
case null: boolBuilder.AppendNull(); break;
case bool boolValue:
boolBuilder.Append(boolValue); break;
- default:
boolBuilder.Append(ConvertValue(param.Value, Convert.ToBoolean,
DbType.Boolean)); break;
+ default:
boolBuilder.Append(ConvertValue(value, Convert.ToBoolean, DbType.Boolean));
break;
}
parameters[i] = boolBuilder.Build();
break;
case DbType.Byte:
var uint8Builder = new UInt8Array.Builder();
- switch (param.Value)
+ switch (value)
{
case null: uint8Builder.AppendNull(); break;
case byte byteValue:
uint8Builder.Append(byteValue); break;
- default:
uint8Builder.Append(ConvertValue(param.Value, Convert.ToByte, DbType.Byte));
break;
+ default:
uint8Builder.Append(ConvertValue(value, Convert.ToByte, DbType.Byte)); break;
}
parameters[i] = uint8Builder.Build();
break;
case DbType.Date:
var dateBuilder = new Date32Array.Builder();
- switch (param.Value)
+ switch (value)
{
case null: dateBuilder.AppendNull(); break;
case DateTime datetime:
dateBuilder.Append(datetime); break;
#if NET5_0_OR_GREATER
case DateOnly dateonly:
dateBuilder.Append(dateonly); break;
#endif
- default:
dateBuilder.Append(ConvertValue(param.Value, Convert.ToDateTime, DbType.Date));
break;
+ default:
dateBuilder.Append(ConvertValue(value, Convert.ToDateTime, DbType.Date)); break;
}
parameters[i] = dateBuilder.Build();
break;
case DbType.DateTime:
var timestampBuilder = new
TimestampArray.Builder();
- switch (param.Value)
+ switch (value)
{
case null: timestampBuilder.AppendNull();
break;
case DateTime datetime:
timestampBuilder.Append(datetime); break;
- default:
timestampBuilder.Append(ConvertValue(param.Value, Convert.ToDateTime,
DbType.DateTime)); break;
+ default:
timestampBuilder.Append(ConvertValue(value, Convert.ToDateTime,
DbType.DateTime)); break;
}
parameters[i] = timestampBuilder.Build();
break;
+ case DbType.DateTimeOffset:
+ // Bound as a UTC timestamp, which every driver
understands; the
+ // original offset is not preserved.
+ var timestampOffsetBuilder = new
TimestampArray.Builder(TimeUnit.Microsecond, "UTC");
+ switch (value)
+ {
+ case null:
timestampOffsetBuilder.AppendNull(); break;
+ case DateTimeOffset dateTimeOffset:
timestampOffsetBuilder.Append(dateTimeOffset); break;
+ case DateTime datetime:
timestampOffsetBuilder.Append(new DateTimeOffset(datetime)); break;
+ default:
timestampOffsetBuilder.Append(ConvertValue(value, v =>
DateTimeOffset.Parse(Convert.ToString(v, CultureInfo.InvariantCulture)!,
CultureInfo.InvariantCulture), DbType.DateTimeOffset)); break;
+ }
+ parameters[i] = timestampOffsetBuilder.Build();
+ break;
case DbType.Decimal:
- var value = param.Value switch
+ var decimalValue = value switch
{
null => (SqlDecimal?)null,
SqlDecimal sqlDecimal => sqlDecimal,
decimal d => new SqlDecimal(d),
- _ => new SqlDecimal(ConvertValue(param.Value,
Convert.ToDecimal, DbType.Decimal)),
+ _ => new SqlDecimal(ConvertValue(value,
Convert.ToDecimal, DbType.Decimal)),
};
- var decimalBuilder = new
Decimal128Array.Builder(new Decimal128Type(value?.Precision ?? 10, value?.Scale
?? 0));
- if (value is null)
+ var decimalBuilder = new
Decimal128Array.Builder(new Decimal128Type(decimalValue?.Precision ?? 10,
decimalValue?.Scale ?? 0));
+ if (decimalValue is null)
{
decimalBuilder.AppendNull();
}
else
{
- decimalBuilder.Append(value.Value);
+ decimalBuilder.Append(decimalValue.Value);
}
parameters[i] = decimalBuilder.Build();
break;
case DbType.Double:
var doubleBuilder = new DoubleArray.Builder();
- switch (param.Value)
+ switch (value)
{
case null: doubleBuilder.AppendNull(); break;
case double dbl: doubleBuilder.Append(dbl);
break;
- default:
doubleBuilder.Append(ConvertValue(param.Value, Convert.ToDouble,
DbType.Double)); break;
+ default:
doubleBuilder.Append(ConvertValue(value, Convert.ToDouble, DbType.Double));
break;
}
parameters[i] = doubleBuilder.Build();
break;
+ case DbType.Guid:
+ var guidBuilder = new GuidArray.Builder();
+ switch (value)
+ {
+ case null: guidBuilder.AppendNull(); break;
+ case Guid guid: guidBuilder.Append(guid);
break;
+ case byte[] bytes when bytes.Length == 16:
guidBuilder.Append(new Guid(bytes)); break;
Review Comment:
`GuidArray` stores UUID bytes in RFC 4122 big-endian order, but `new
Guid(bytes)` interprets the first three fields using .NET's legacy
little-endian layout. A 16-byte canonical `arrow.uuid` parameter will therefore
be byte-swapped before binding (for example, the bytes returned by
`GuidArray.GuidToBytes(guid)` will not bind back to `guid`). Convert canonical
bytes with `GuidArray.RFC4122ToGuid(bytes)` instead.
##########
csharp/src/Client/SchemaConverter.cs:
##########
@@ -214,6 +215,23 @@ public static Type GetArrowType(Field f, DecimalBehavior
decimalBehavior, Struct
}
goto default;
+ case ArrowTypeId.Extension:
+ switch (f.DataType)
+ {
+ case GuidType:
+ return typeof(Guid);
+ case Bool8Type:
+ return typeof(bool);
+ case TimestampWithOffsetType:
+ return typeof(DateTimeOffset);
+ case VariantType:
+ return structBehavior == StructBehavior.JsonString
? typeof(string) : typeof(object);
+ default:
+ // Unrecognized extension types are surfaced as
their storage values.
+ ExtensionType extensionType =
(ExtensionType)f.DataType;
+ return ConvertArrowType(new Field(f.Name,
extensionType.StorageType, f.IsNullable), decimalBehavior, structBehavior);
Review Comment:
The new nested extension resolution can produce a ListType whose value type
is GuidType, Bool8Type, TimestampWithOffsetType, or VariantType, but
ConvertArrowType still routes lists through GetArrowArrayType(valueType), whose
switch has no extension cases. Calling GetFieldType, GetColumnSchema, or
GetSchemaTable for a list of one of these extensions therefore throws
InvalidCastException; update list/array-type conversion to handle resolved
extension children.
##########
csharp/src/Client/ExtensionTypes.cs:
##########
@@ -0,0 +1,259 @@
+/*
+* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+using System;
+using System.Collections.Generic;
+using Apache.Arrow.Adbc.Extensions;
+using Apache.Arrow.Operations.Shredding;
+using Apache.Arrow.Operations.VariantJson;
+using Apache.Arrow.Scalars.Variant;
+using Apache.Arrow.Types;
+
+namespace Apache.Arrow.Adbc.Client
+{
+ /// <summary>
+ /// Support for Arrow extension types in the ADO.NET client.
+ /// </summary>
+ internal static class ExtensionTypes
+ {
+ internal const string ExtensionNameKey = "ARROW:extension:name";
+ internal const string ExtensionMetadataKey =
"ARROW:extension:metadata";
+
+ private static readonly DateTimeOffset s_unixEpoch = new
DateTimeOffset(1970, 1, 1, 0, 0, 0, TimeSpan.Zero);
+ private static readonly DateTime s_unixEpochUnspecified = new
DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Unspecified);
+
+ /// <summary>
+ /// The extension types the client understands without any
registration by the caller.
+ /// </summary>
+ private static readonly ExtensionTypeRegistry s_builtIn =
CreateBuiltInRegistry();
+
+ private static ExtensionTypeRegistry CreateBuiltInRegistry()
+ {
+ ExtensionTypeRegistry registry = new ExtensionTypeRegistry();
+ registry.Register(GuidExtensionDefinition.Instance);
+ registry.Register(Bool8ExtensionDefinition.Instance);
+ registry.Register(TimestampWithOffsetExtensionDefinition.Instance);
+ registry.Register(VariantExtensionDefinition.Instance);
+ return registry;
+ }
+
+ /// <summary>
+ /// Arrays imported through the C Data Interface carry extension types
only as field
+ /// metadata over their storage type. Returns a field whose type is
the corresponding
+ /// <see cref="ExtensionType"/> when the extension is known, or the
original field otherwise.
+ /// Fields nested in struct and list types are resolved too.
+ /// Definitions registered with <see
cref="ExtensionTypeRegistry.Default"/> take precedence
+ /// over the built-in ones.
+ /// </summary>
+ public static Field Resolve(Field field)
+ {
+ if (field.DataType is ExtensionType)
+ {
+ return field;
+ }
+
+ IArrowType resolvedType = ResolveChildren(field.DataType);
+ if (!ReferenceEquals(resolvedType, field.DataType))
+ {
+ field = new Field(field.Name, resolvedType, field.IsNullable,
field.Metadata);
+ }
+
+ if (!field.HasMetadata ||
+ !field.Metadata.TryGetValue(ExtensionNameKey, out string?
name))
+ {
+ return field;
+ }
+
+ if (!ExtensionTypeRegistry.Default.TryGetDefinition(name, out
ExtensionDefinition definition) &&
+ !s_builtIn.TryGetDefinition(name, out definition))
+ {
+ return field;
+ }
+
+ field.Metadata.TryGetValue(ExtensionMetadataKey, out string?
metadata);
+ if (!definition.TryCreateType(field.DataType, metadata ??
string.Empty, out ExtensionType extensionType))
+ {
+ return field;
+ }
+
+ return new Field(field.Name, extensionType, field.IsNullable,
field.Metadata);
+ }
+
+ private static IArrowType ResolveChildren(IArrowType type)
+ {
+ if (type is not NestedType nestedType)
+ {
+ return type;
+ }
+
+ Field[] fields = new Field[nestedType.Fields.Count];
+ bool changed = false;
+ for (int i = 0; i < fields.Length; i++)
+ {
+ fields[i] = Resolve(nestedType.Fields[i]);
+ changed |= !ReferenceEquals(fields[i], nestedType.Fields[i]);
+ }
+
+ if (!changed)
+ {
+ return type;
+ }
+
+ switch (type)
+ {
+ case StructType:
+ return new StructType(fields);
+ case ListType:
+ return new ListType(fields[0]);
+ case LargeListType:
+ return new LargeListType(fields[0]);
+ case FixedSizeListType fixedSizeListType:
+ return new FixedSizeListType(fields[0],
fixedSizeListType.ListSize);
Review Comment:
Resolving nested UUID/bool8/other extensions creates types such as
`ListType(GuidType)`, but `SchemaConverter.ConvertArrowType` sends every list
value type to `GetArrowArrayType`, which has no extension cases. Consequently a
reader can read the list value but `GetFieldType`, `GetColumnSchema`, or
`GetSchemaTable` throws for `list<uuid>` (and the other extension element
types). Add schema mappings for extension element arrays (and their storage
fallback) and cover the schema APIs in the nested-extension test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
