CurtHagenlocher commented on code in PR #4919:
URL: https://github.com/apache/arrow-adbc/pull/4919#discussion_r4307791230
##########
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:
I don't think this one applies: `SchemaConverter.cs` already has `using
System.Collections.Generic;` (line 19), and the client project builds for both
`netstandard2.0` and `net8.0`.
##########
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:
Fixed in a184940b5. Variant conversion moved from the client into the core
`IArrowArrayExtensions` (new `VariantConversions`), so `ValueAt` and
`GetValueConverter` handle `VariantArray` everywhere, including inside
`ParseStructArray`. A nested variant now becomes the same
`Dictionary`/`List`/scalar as a top-level one under Strict, and is serialized
as part of the struct's JSON under JsonString. As a result, the
`Apache.Arrow.Operations` reference moved from the client to the core library.
That adds a 48 KB assembly but no new transitive packages, since core already
depends on `Apache.Arrow`/`Scalars` and `System.Text.Json`. The NativeAOT
fixture compiles without trim/AOT warnings. `NestedVariant` covers both
behaviors.
##########
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:
Good catch, fixed in a184940b5. The bytes are now read with
`GuidArray.RFC4122ToGuid`. `BindGuidAndDateTimeOffsetParameters` now binds
`GuidArray.GuidToBytes(guid)` and checks that it comes back as `guid`.
##########
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:
Fixed in a184940b5. `GetArrowArrayType` now maps uuid, bool8,
timestamp_with_offset and variant element types to their array classes, other
extension types to `ExtensionArray`, and `fixed_size_binary` to
`FixedSizeBinaryArray`. `NestedGuidFromStorageWithExtensionMetadata` now also
checks `GetFieldType`, `GetColumnSchema` and `GetSchemaTable` for the
`list<uuid>` column.
##########
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:
Same issue as the comment on `ExtensionTypes.cs`; fixed in a184940b5 as
described there.
--
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]
