eerhardt commented on code in PR #34133: URL: https://github.com/apache/arrow/pull/34133#discussion_r1148397028
########## csharp/test/Apache.Arrow.Tests/CDataInterfaceSchemaTests.cs: ########## @@ -0,0 +1,427 @@ +// 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 System.Linq; +using System.Runtime.InteropServices; +using Apache.Arrow.C; +using Apache.Arrow.Types; +using Python.Runtime; +using Xunit; + +namespace Apache.Arrow.Tests +{ + public class CDataSchemaTest + { + [Fact] + public unsafe void InitializeZeroed() + { + CArrowSchema* cSchema = CArrowSchema.Create(); + + Assert.True(cSchema->format == null); + Assert.True(cSchema->name == null); + Assert.True(cSchema->metadata == null); + Assert.Equal(0, cSchema->flags); + Assert.Equal(0, cSchema->n_children); + Assert.True(cSchema->children == null); + Assert.True(cSchema->dictionary == null); + Assert.True(cSchema->release == null); + Assert.True(cSchema->private_data == null); + + CArrowSchema.Free(cSchema); + } + + [Fact] + public unsafe void FlagsSet() + { + // Non-nullable field + { + var nonNullField = new Field("non_null", Int32Type.Default, false); + CArrowSchema* cSchema = CArrowSchema.Create(); + CArrowSchemaExporter.ExportField(nonNullField, cSchema); + Assert.False(cSchema->GetFlag(CArrowSchema.ArrowFlagNullable)); + CArrowSchema.Free(cSchema); + } + + // Nullable field + { + var nullableField = new Field("nullable", Int32Type.Default, true); + CArrowSchema* cSchema = CArrowSchema.Create(); + CArrowSchemaExporter.ExportField(nullableField, cSchema); + Assert.True(cSchema->GetFlag(CArrowSchema.ArrowFlagNullable)); + CArrowSchema.Free(cSchema); + } + + // dictionary ordered + { + var orderedDictionary = new DictionaryType(Int32Type.Default, StringType.Default, true); + CArrowSchema* cSchema = CArrowSchema.Create(); + CArrowSchemaExporter.ExportType(orderedDictionary, cSchema); + Assert.True(cSchema->GetFlag(CArrowSchema.ArrowFlagDictionaryOrdered)); + CArrowSchema.Free(cSchema); + } + + // dictionary unordered + { + var unorderedDictionary = new DictionaryType(Int32Type.Default, StringType.Default, false); + CArrowSchema* cSchema = CArrowSchema.Create(); + CArrowSchemaExporter.ExportType(unorderedDictionary, cSchema); + Assert.False(cSchema->GetFlag(CArrowSchema.ArrowFlagDictionaryOrdered)); + CArrowSchema.Free(cSchema); + } + } + + [Fact] + public unsafe void CallsReleaseForValid() + { + CArrowSchema* cSchema = CArrowSchema.Create(); + CArrowSchemaExporter.ExportType(Int32Type.Default, cSchema); + Assert.False(cSchema->release == null); + CArrowSchemaImporter.ImportType(cSchema); + Assert.True(cSchema->release == null); + CArrowSchema.Free(cSchema); + } + + [Fact] + public unsafe void CallsReleaseForInvalid() + { + // Make sure we call release callback, even if the imported schema + // is invalid. + CArrowSchema* cSchema = CArrowSchema.Create(); + + bool wasCalled = false; + var releaseCallback = (CArrowSchema* cSchema) => + { + wasCalled = true; + cSchema->release = null; + }; + cSchema->release = (delegate* unmanaged[Stdcall]<CArrowSchema*, void>)Marshal.GetFunctionPointerForDelegate( + releaseCallback); + + Assert.Throws<NullReferenceException>(() => + { + CArrowSchemaImporter.ImportType(cSchema); + }); + Assert.True(wasCalled); + CArrowSchema.Free(cSchema); + } + } + + public class CDataSchemaPythonTest Review Comment: Can we split this into a new file? Having a single class per file makes it easier to navigate the code and find what you are looking for. ########## csharp/src/Apache.Arrow/C/CArrowSchemaExporter.cs: ########## @@ -0,0 +1,278 @@ +// 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 System.IO; +using System.Linq; +using System.Runtime.InteropServices; +using Apache.Arrow.Types; + +namespace Apache.Arrow.C +{ + public static class CArrowSchemaExporter + { + /// <summary> + /// Export a type to a <see cref="CArrowSchema"/>. + /// </summary> + /// <param name="datatype">The datatype to export</param> + /// <param name="schema">An allocated but uninitialized CArrowSchema pointer.</param> + /// <example> + /// <code> + /// CArrowSchema* exportPtr = CArrowSchema.Create(); + /// CArrowSchemaExporter.ExportType(dataType, exportPtr); + /// foreign_import_function(exportPtr); + /// CArrowSchema.Free(exportPtr); + /// </code> + /// </example> + public static unsafe void ExportType(IArrowType datatype, CArrowSchema* schema) + { + if (datatype == null) + { + throw new ArgumentNullException(nameof(datatype)); + } + if (schema == null) + { + throw new ArgumentNullException(nameof(schema)); + } + if (schema->release != null) + { + throw new ArgumentException("Cannot export schema to a struct that is already initialized."); + } + + schema->format = StringUtil.ToCStringUtf8(GetFormat(datatype)); + schema->name = null; + schema->metadata = null; + schema->flags = GetFlags(datatype); + + schema->children = ConstructChildren(datatype, out var numChildren); + schema->n_children = numChildren; + + schema->dictionary = ConstructDictionary(datatype); + + schema->release = (delegate* unmanaged[Stdcall]<CArrowSchema*, void>)Marshal.GetFunctionPointerForDelegate( + ReleaseCArrowSchema); + + schema->private_data = null; + } + + /// <summary> + /// Export a field to a <see cref="CArrowSchema"/>. + /// </summary> + /// <param name="field">The field to export</param> + /// <param name="schema">An allocated but uninitialized CArrowSchema pointer.</param> + /// <example> + /// <code> + /// CArrowSchema* exportPtr = CArrowSchema.Create(); + /// CArrowSchemaExporter.ExportType(field, exportPtr); + /// foreign_import_function(exportPtr); + /// CArrowSchema.Free(exportPtr); + /// </code> + /// </example> + public static unsafe void ExportField(Field field, CArrowSchema* schema) + { + ExportType(field.DataType, schema); + schema->name = StringUtil.ToCStringUtf8(field.Name); + // TODO: field metadata Review Comment: Can you log issues for these TODOs? ########## csharp/src/Apache.Arrow/C/CArrowSchema.cs: ########## @@ -0,0 +1,124 @@ +// 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.Runtime.InteropServices; +using Apache.Arrow.Types; + +namespace Apache.Arrow.C +{ + /// <summary> + /// An Arrow C Data Interface Schema, which represents a type, field, or schema. + /// </summary> + /// <remarks> + /// This is used to export <see cref="ArrowType"/>, <see cref="Field"/>, or + /// <see cref="Schema"/> to other languages. It matches the layout of the + /// ArrowSchema struct described in https://github.com/apache/arrow/blob/main/cpp/src/arrow/c/abi.h. + /// </remarks> + [StructLayout(LayoutKind.Sequential)] + public unsafe struct CArrowSchema + { + public byte* format; + public byte* name; + public byte* metadata; + public long flags; + public long n_children; + public CArrowSchema** children; + public CArrowSchema* dictionary; + public delegate* unmanaged[Stdcall]<CArrowSchema*, void> release; Review Comment: In the future (when we start targeting net6.0+), I think we will want to drop the `[Stdcall]` here and just use the default: ``` public delegate* unmanaged<CArrowSchema*, void> release; ``` But that doesn't work in the older TFMs, so what we have now is fine. -- 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]
