[GitHub] [avro] zcsizmadia commented on a change in pull request #1597: AVRO-2211: SchemaBuilder equivalent or other means of schema creation

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

zcsizmadia commented on a change in pull request #1597:
URL: https://github.com/apache/avro/pull/1597#discussion_r841129065



##########
File path: lang/csharp/src/apache/main/Schema/MapSchema.cs
##########
@@ -36,10 +36,11 @@ public class MapSchema : UnnamedSchema
         /// Creates a new <see cref="MapSchema"/> from the given schema.
         /// </summary>
         /// <param name="type">Schema to create the map schema from.</param>
+        /// <param name="cutsomProperties">Dictionary that provides access to custom properties</param>
         /// <returns>A new <see cref="MapSchema"/>.</returns>
-        public static MapSchema CreateMap(Schema type)
+        public static MapSchema CreateMap(Schema type, PropertyMap cutsomProperties = null)

Review comment:
       Fix spelling plz

##########
File path: lang/csharp/src/apache/main/Schema/EnumSchema.cs
##########
@@ -103,15 +133,46 @@ internal static EnumSchema NewInstance(JToken jtok, PropertyMap props, SchemaNam
                             string doc, string defaultSymbol)
                             : base(Type.Enumeration, name, aliases, props, names, doc)
         {
-            if (null == name.Name) throw new SchemaParseException("name cannot be null for enum schema.");
+            if (null == name.Name) throw new AvroException("name cannot be null for enum schema.");
             this.Symbols = symbols;
             this.symbolMap = symbolMap;
 
             if (null != defaultSymbol && !symbolMap.ContainsKey(defaultSymbol))
-                throw new SchemaParseException($"Default symbol: {defaultSymbol} not found in symbols");
+                throw new AvroException($"Default symbol: {defaultSymbol} not found in symbols");
             Default = defaultSymbol;
         }
 
+        /// <summary>
+        /// Creates symbols map from specified list of symbols.
+        /// Symbol map contains the names of the symbols and their index.
+        /// </summary>
+        /// <param name="symbols">List of symbols</param>
+        /// <returns>Symbol map</returns>
+        /// <exception cref="AvroException">Is thrown if the symbols list contains invalid symbol name or duplicate symbols</exception>
+        private static IDictionary<string, int> CreateSymbolsMap(IEnumerable<string> symbols)
+        {
+            IDictionary<string, int> symbolMap = new Dictionary<string, int>();
+            int i = 0;
+            foreach (var symbol in symbols)
+            {
+                if (ValidateSymbolName(symbol))
+                {
+                    throw new AvroException($"Invalid symbol name: {symbol}");
+                }
+
+                if (symbolMap.ContainsKey(symbol))
+                {
+                    throw new AvroException($"Duplicate symbol: {symbol}");
+                }
+
+                symbolMap[symbol] = i++;
+            }
+
+            return symbolMap;
+        }
+
+        private static bool ValidateSymbolName(string symbol) => string.IsNullOrEmpty(symbol) || !Regex.IsMatch(symbol, "^([A-Za-z_][A-Za-z0-9_]*)$");

Review comment:
       I always have perfromance concerns when we use Regex. I know that JsonHelper.GetRequiredString(), however is that a concern here? What I am thinking is that when someone is using a schemaBuilder in runtime, this call might be called many times? I might be just too paranoid here. In a unit test I have no concern, however in the main library, it might cause unnecessary delay for a simple check. Of course if the name might be more complicated in the future, regex is probably the easiest/best solution.
   
   IMO ValidateSymbol should throw the exception if not valid, something like this:
   
   ```
   private static void ValidateSymbolName(string symbol)
   {
       if (string.IsNullOrEmpty(symbol) || !Regex.IsMatch(symbol, "^([A-Za-z_][A-Za-z0-9_]*)$"))
       {
           throw new AvroException($"Invalid symbol name: {symbol}");
       }
   }
   ```
   




-- 
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: issues-unsubscribe@avro.apache.org

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