[GitHub] [arrow] eerhardt commented on a change in pull request #8902: ARROW-10719: [C#] ArrowStreamWriter doesn't write schema metadata

[GitBox <gi...@apache.org>]

eerhardt commented on a change in pull request #8902:
URL: https://github.com/apache/arrow/pull/8902#discussion_r542736853



##########
File path: csharp/src/Apache.Arrow/Field.cs
##########
@@ -34,6 +35,22 @@ public partial class Field
 
         public Field(string name, IArrowType dataType, bool nullable,
             IEnumerable<KeyValuePair<string, string>> metadata = default)
+            : this(name, dataType, nullable)
+        {
+            Metadata = metadata?.ToDictionary(kv => kv.Key, kv => kv.Value);
+
+        }
+
+        internal Field(string name, IArrowType dataType, bool nullable,
+            IReadOnlyDictionary<string, string> metadata, bool copyCollections)

Review comment:
       The `copyCollections` is not in the private constructor with `if (copyCollections)` because the parameter types wouldn't line up.
   
   The public ctor takes `IEnumerable<KeyValuePair<string, string>> metadata`. So it can't directly pass `metadata` to a private constructor that would take `IReadOnlyDictionary<string, string>`. It would need to call `ToDictionary` on it, and it would no longer need to pass `true` to `copyCollections`.
   
   The private constructor can't take `IEnumerable<KeyValuePair<string, string>> metadata` as a parameter because:
   
   1. It would conflict with the public ctor - they would have the same exact signature.
   2. It couldn't directly set the `public IReadOnlyDictionary<string, string> Metadata { get; }` property without a cast.
   
   
   Getting these constructors working correctly is a bit tricky. I couldn't just make an internal ctor:
   
   `internal Field(string name, IArrowType dataType, bool nullable, IReadOnlyDictionary<string, string> metadata)`
   
   because now internal code (like the Field.Builder) will start binding to the "non-copy" ctor, which is wrong.
   
   So the best option I figured out was to make another parameter to disambiguate which ctor was being called, and it expects anyone using this new ctor only ever wants to not copy the collections, or else they should use the public constructor.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org