This is an automated email from the ASF dual-hosted git repository. CurtHagenlocher pushed a commit to branch ipc-message-custom-metadata in repository https://gitbox.apache.org/repos/asf/arrow-dotnet.git
commit fcff0b807133927e9cd9c27e31477aab34645c6b Author: Rusty Conover <[email protected]> AuthorDate: Mon Aug 24 15:25:36 2026 -0400 Address Copilot review comments on #424 - FlightDataStream: override the customMetadata-aware WriteMessageAsync overload (the one WriteRecordBatchInternalAsync now actually calls) instead of the old 4-arg overload, so Flight writes aren't silently routed through the base implementation and don't bypass DataHeader capture. - ArrowStreamWriter: validate that caller-supplied custom metadata has no null keys/values before building FlatBuffer offsets, so failures are reported as a clear ArgumentException rather than an opaque FlatBufferBuilder exception. - ArrowStreamReader: correct the LastBatchCustomMetadata XML doc to describe its actual update semantics (it's left unchanged when a read call returns null, e.g. at end of stream). - CustomMetadataPythonTests: use the repo's shared PythonNetFixture + [Collection("PythonNet")] instead of a private per-class Python.NET init/shutdown, avoiding double-Initialize/premature-Shutdown races with other Python.NET tests; this also fixes the missing Py.GIL() guard around the Windows sys.path append, since the shared fixture already wraps that in using (Py.GIL()). Verified with: - dotnet build Apache.Arrow.sln — 0 warnings/errors - dotnet test test/Apache.Arrow.Tests/Apache.Arrow.Tests.csproj — 1876 passed, 30 skipped, 0 failed - dotnet format Apache.Arrow.sln --exclude src/Apache.Arrow/Flatbuf/FlatBuffers/ --verify-no-changes — clean --- .../Internal/FlightDataStream.cs | 4 +- src/Apache.Arrow/Ipc/ArrowStreamReader.cs | 6 ++- src/Apache.Arrow/Ipc/ArrowStreamWriter.cs | 21 ++++++++ .../CustomMetadataPythonTests.cs | 59 ++-------------------- 4 files changed, 31 insertions(+), 59 deletions(-) diff --git a/src/Apache.Arrow.Flight/Internal/FlightDataStream.cs b/src/Apache.Arrow.Flight/Internal/FlightDataStream.cs index 50b2a40..3ab4331 100644 --- a/src/Apache.Arrow.Flight/Internal/FlightDataStream.cs +++ b/src/Apache.Arrow.Flight/Internal/FlightDataStream.cs @@ -91,11 +91,11 @@ namespace Apache.Arrow.Flight.Internal await _clientStreamWriter.WriteAsync(_currentFlightData).ConfigureAwait(false); } - private protected override ValueTask<long> WriteMessageAsync<T>(MessageHeader headerType, Offset<T> headerOffset, int bodyLength, CancellationToken cancellationToken) + private protected override ValueTask<long> WriteMessageAsync<T>(MessageHeader headerType, Offset<T> headerOffset, int bodyLength, VectorOffset customMetadataOffset, CancellationToken cancellationToken) { Offset<Flatbuf.Message> messageOffset = Flatbuf.Message.CreateMessage( Builder, CurrentMetadataVersion, headerType, headerOffset.Value, - bodyLength); + bodyLength, customMetadataOffset); Builder.Finish(messageOffset.Value); diff --git a/src/Apache.Arrow/Ipc/ArrowStreamReader.cs b/src/Apache.Arrow/Ipc/ArrowStreamReader.cs index 8379b12..e7b5861 100644 --- a/src/Apache.Arrow/Ipc/ArrowStreamReader.cs +++ b/src/Apache.Arrow/Ipc/ArrowStreamReader.cs @@ -155,8 +155,10 @@ namespace Apache.Arrow.Ipc /// <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. + /// Set whenever ReadNextRecordBatch/ReadNextRecordBatchAsync successfully reads a + /// RecordBatch message; left unchanged when a call returns null (e.g. at the end of + /// the stream), so it continues to reflect the last RecordBatch that was read. + /// Returns null if that batch had no custom metadata. /// </summary> public IReadOnlyDictionary<string, string> LastBatchCustomMetadata => _implementation.LastBatchCustomMetadata; } diff --git a/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs b/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs index a13725f..c8e6032 100644 --- a/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs +++ b/src/Apache.Arrow/Ipc/ArrowStreamWriter.cs @@ -838,6 +838,7 @@ namespace Apache.Arrow.Ipc VectorOffset customMetadataVectorOffset = default; if (customMetadata != null && customMetadata.Count > 0) { + ValidateCustomMetadata(customMetadata); Offset<Flatbuf.KeyValue>[] metadataOffsets = GetMetadataOffsets(customMetadata); customMetadataVectorOffset = Flatbuf.Message.CreateCustomMetadataVector(Builder, metadataOffsets); } @@ -894,6 +895,7 @@ namespace Apache.Arrow.Ipc VectorOffset customMetadataVectorOffset = default; if (customMetadata != null && customMetadata.Count > 0) { + ValidateCustomMetadata(customMetadata); Offset<Flatbuf.KeyValue>[] metadataOffsets = GetMetadataOffsets(customMetadata); customMetadataVectorOffset = Flatbuf.Message.CreateCustomMetadataVector(Builder, metadataOffsets); } @@ -1330,6 +1332,25 @@ namespace Apache.Arrow.Ipc return Flatbuf.DictionaryEncoding.CreateDictionaryEncoding(Builder, id, indexOffset, dicType.Ordered); } + /// <summary> + /// Validates that a caller-supplied custom metadata dictionary contains no null keys or values, + /// so that failures are reported clearly rather than as an opaque exception from the FlatBuffer builder. + /// </summary> + private static void ValidateCustomMetadata(IReadOnlyDictionary<string, string> customMetadata) + { + foreach (KeyValuePair<string, string> metadatum in customMetadata) + { + if (metadatum.Key == null) + { + throw new ArgumentException("Custom metadata must not contain null keys.", nameof(customMetadata)); + } + if (metadatum.Value == null) + { + throw new ArgumentException($"Custom metadata value for key '{metadatum.Key}' must not be null.", nameof(customMetadata)); + } + } + } + private Offset<Flatbuf.KeyValue>[] GetMetadataOffsets(IReadOnlyDictionary<string, string> metadata) { Debug.Assert(metadata != null); diff --git a/test/Apache.Arrow.Tests/CustomMetadataPythonTests.cs b/test/Apache.Arrow.Tests/CustomMetadataPythonTests.cs index 56f68f3..2231723 100644 --- a/test/Apache.Arrow.Tests/CustomMetadataPythonTests.cs +++ b/test/Apache.Arrow.Tests/CustomMetadataPythonTests.cs @@ -27,63 +27,12 @@ namespace Apache.Arrow.Tests // Cross-language Python tests for custom_metadata // ------------------------------------------------------------------- - public class CustomMetadataPythonTests : IClassFixture<CustomMetadataPythonTests.PythonNet> + [Collection("PythonNet")] + public class CustomMetadataPythonTests { - public class PythonNet : IDisposable + public CustomMetadataPythonTests(PythonNetFixture pythonNet) { - 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")); - } - - Initialized = true; - } - - public void Dispose() - { - PythonEngine.Shutdown(); - } - } - - public CustomMetadataPythonTests(PythonNet pythonNet) - { - if (!pythonNet.Initialized) - { - var errorReason = pythonNet.VersionMismatch ? "Python version is incompatible with PythonNet" : "PYTHONNET_PYDLL not set"; - - bool inCIJob = Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true"; - bool inVerificationJob = Environment.GetEnvironmentVariable("TEST_CSHARP") == "1"; - - Skip.If(inVerificationJob || !inCIJob, $"{errorReason}; skipping custom metadata Python tests."); - - throw new Exception($"{errorReason}; cannot run custom metadata Python tests."); - } + pythonNet.EnsureInitialized(); } // -------------------------------------------------------------------
