Copilot commented on code in PR #424:
URL: https://github.com/apache/arrow-dotnet/pull/424#discussion_r3846602044
##########
src/Apache.Arrow/Ipc/ArrowStreamWriter.cs:
##########
@@ -882,6 +910,7 @@ private protected async Task
WriteRecordBatchInternalAsync(RecordBatch recordBat
long metadataLength = await
WriteMessageAsync(Flatbuf.MessageHeader.RecordBatch,
recordBatchOffset, recordBatchBuilder.TotalLength,
+ customMetadataVectorOffset,
cancellationToken).ConfigureAwait(false);
Review Comment:
`WriteRecordBatchInternalAsync` now always calls the new
`WriteMessageAsync(..., VectorOffset customMetadataOffset, ...)` overload. This
bypasses existing overrides of the original `WriteMessageAsync(...,
CancellationToken)` (e.g., in
`Apache.Arrow.Flight/Internal/FlightDataStream.cs`), changing behavior for all
async record batch writes even when no custom metadata is provided.
##########
src/Apache.Arrow/Ipc/ArrowStreamReader.cs:
##########
@@ -151,5 +152,12 @@ public RecordBatch ReadNextRecordBatch()
{
return _implementation.ReadNextRecordBatch();
}
+
+ /// <summary>
+ /// Custom metadata from the most recently read RecordBatch Message.
+ /// Updated after each call to
ReadNextRecordBatch/ReadNextRecordBatchAsync.
+ /// Returns null if the last batch had no custom metadata.
+ /// </summary>
Review Comment:
The XML docs say `LastBatchCustomMetadata` is "Updated after each call to
ReadNextRecordBatch/ReadNextRecordBatchAsync", but the implementation only
updates it when a RecordBatch message is successfully read. If
`ReadNextRecordBatch*` returns null at end-of-stream, the property remains
whatever it was for the prior batch. The docs should reflect the actual update
semantics (or the implementation should clear the property on null).
##########
src/Apache.Arrow/Ipc/ArrowStreamWriter.cs:
##########
@@ -829,6 +834,14 @@ private protected void
WriteRecordBatchInternal(RecordBatch recordBatch)
VectorOffset buffersVectorOffset = Builder.EndVector();
+ // Build custom metadata for the Message if provided
+ VectorOffset customMetadataVectorOffset = default;
+ if (customMetadata != null && customMetadata.Count > 0)
+ {
+ Offset<Flatbuf.KeyValue>[] metadataOffsets =
GetMetadataOffsets(customMetadata);
+ customMetadataVectorOffset =
Flatbuf.Message.CreateCustomMetadataVector(Builder, metadataOffsets);
Review Comment:
The new `customMetadata` path forwards directly into `GetMetadataOffsets`,
which calls `Builder.CreateString(metadatum.Key)` /
`Builder.CreateString(metadatum.Value)` and will throw if callers provide null
keys or values. Since this is a newly public-facing API surface, consider
validating inputs (or documenting that null keys/values are not supported) so
failures are predictable and actionable.
##########
test/Apache.Arrow.Tests/CustomMetadataPythonTests.cs:
##########
@@ -0,0 +1,183 @@
+// 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 Apache.Arrow.Ipc;
+using Python.Runtime;
+using Xunit;
+
+namespace Apache.Arrow.Tests
+{
+
+ // -------------------------------------------------------------------
+ // Cross-language Python tests for custom_metadata
+ // -------------------------------------------------------------------
+
+ public class CustomMetadataPythonTests :
IClassFixture<CustomMetadataPythonTests.PythonNet>
+ {
+ public class PythonNet : IDisposable
Review Comment:
This test class introduces a new, per-class Python.NET initialization
fixture (`PythonEngine.Initialize`/`Shutdown`) but the repo already centralizes
Python.NET lifecycle in `PythonNetFixture` + `[Collection("PythonNet")]` (see
`PythonNetFixture.cs` / `PythonNetCollection.cs`). Not using the shared
collection risks parallel test execution calling `Initialize` multiple times
and/or one class calling `Shutdown` while another Python test is still running.
This issue also appears on line 68 of the same file.
##########
test/Apache.Arrow.Tests/CustomMetadataPythonTests.cs:
##########
@@ -0,0 +1,183 @@
+// 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 Apache.Arrow.Ipc;
+using Python.Runtime;
+using Xunit;
+
+namespace Apache.Arrow.Tests
+{
+
+ // -------------------------------------------------------------------
+ // Cross-language Python tests for custom_metadata
+ // -------------------------------------------------------------------
+
+ public class CustomMetadataPythonTests :
IClassFixture<CustomMetadataPythonTests.PythonNet>
+ {
+ public class PythonNet : IDisposable
+ {
+ public bool Initialized { get; }
+
+ public bool VersionMismatch { get; }
+
+ public PythonNet()
+ {
+ bool pythonSet =
Environment.GetEnvironmentVariable("PYTHONNET_PYDLL") != null;
+ if (!pythonSet)
+ {
+ Initialized = false;
+ return;
+ }
+
+ try
+ {
+ PythonEngine.Initialize();
+ }
+ catch (NotSupportedException e) when
(e.Message.Contains("Python ABI ") && e.Message.Contains("not supported"))
+ {
+ Initialized = false;
+ VersionMismatch = true;
+ return;
+ }
+
+ if
(System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(System.Runtime.InteropServices.OSPlatform.Windows)
&&
+ PythonEngine.PythonPath.IndexOf("dlls",
StringComparison.OrdinalIgnoreCase) < 0)
+ {
+ dynamic sys = Py.Import("sys");
+
sys.path.append(Path.Combine(Path.GetDirectoryName(Environment.GetEnvironmentVariable("PYTHONNET_PYDLL")),
"DLLs"));
+ }
Review Comment:
On Windows, this block calls `Py.Import("sys")` without holding the Python
GIL. Other Python.NET tests in this repo wrap similar code in `using
(Py.GIL())` to avoid unsafe access into the runtime.
--
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]
