[
https://issues.apache.org/jira/browse/AVRO-2211?focusedWorklogId=749381&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-749381
]
ASF GitHub Bot logged work on AVRO-2211:
----------------------------------------
Author: ASF GitHub Bot
Created on: 29/Mar/22 14:47
Start Date: 29/Mar/22 14:47
Worklog Time Spent: 10m
Work Description: zcsizmadia commented on a change in pull request #1597:
URL: https://github.com/apache/avro/pull/1597#discussion_r837531460
##########
File path: lang/csharp/src/apache/main/Schema/Aliases.cs
##########
@@ -0,0 +1,36 @@
+/*
+ * 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
+ *
+ * https://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.Collections.Generic;
+using System.Linq;
+
+namespace Avro
+{
+ internal static class Aliases
+ {
+ internal static IList<SchemaName> GetSchemaNames(IEnumerable<string>
aliases, string enclosingTypeName, string enclosingTypeNamespace)
Review comment:
Could this be IEnumerable<string> and be consistent with the other
Create methods? EnumSchema Create and FixedSchema Create are having an
IEnumerable<string> already.
##########
File path: lang/csharp/src/apache/main/Schema/Aliases.cs
##########
@@ -0,0 +1,36 @@
+/*
+ * 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
+ *
+ * https://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.Collections.Generic;
+using System.Linq;
+
+namespace Avro
+{
+ internal static class Aliases
+ {
+ internal static IList<SchemaName> GetSchemaNames(IEnumerable<string>
aliases, string enclosingTypeName, string enclosingTypeNamespace)
+ {
+ if (aliases == null)
+ {
+ return null;
+ }
+
+ SchemaName enclosingSchemaName = new SchemaName(enclosingTypeName,
enclosingTypeNamespace, null, null);
+ return aliases.Select(alias => new SchemaName(alias,
enclosingSchemaName.Namespace, null, null)).ToList();
Review comment:
If returns IEnumerable<string>, the ToList() conversion is not needed.
##########
File path: lang/csharp/src/apache/main/Schema/RecordSchema.cs
##########
@@ -41,11 +58,111 @@ public class RecordSchema : NamedSchema
/// <summary>
/// Map of field name and Field object for faster field lookups
/// </summary>
- private readonly IDictionary<string, Field> fieldLookup;
+ private IDictionary<string, Field> fieldLookup;
- private readonly IDictionary<string, Field> fieldAliasLookup;
+ private IDictionary<string, Field> fieldAliasLookup;
private bool request;
+ /// <summary>
+ /// Creates a new instance of <see cref="RecordSchema"/>
+ /// </summary>
+ /// <param name="name">name of the record schema</param>
+ /// <param name="fields">list of fields for the record</param>
+ /// <param name="space">type of record schema, either record or
error</param>
+ /// <param name="aliases">list of aliases for the record name</param>
+ /// <param name="customProperties">custom properties on this
schema</param>
+ /// <param name="doc">documentation for this named schema</param>
+ public static RecordSchema Create(string name,
+ List<Field> fields,
+ string space = null,
+ IEnumerable<string> aliases = null,
+ PropertyMap customProperties = null,
+ string doc = null)
+ {
+ return new RecordSchema(Type.Record,
+ new SchemaName(name, space, null, doc),
+ Aliases.GetSchemaNames(aliases, name, space),
+ customProperties,
+ fields,
+ false,
+ CreateFieldMap(fields),
+ CreateFieldMap(fields, true),
+ new SchemaNames(),
+ doc);
+ }
+
+ private static IEnumerable<Schema> EnumerateSchemasRecursive(Schema
schema)
+ {
+ yield return schema;
+ switch (schema.Tag)
+ {
+ case Type.Null:
+ break;
+ case Type.Boolean:
+ break;
+ case Type.Int:
+ break;
+ case Type.Long:
+ break;
+ case Type.Float:
+ break;
+ case Type.Double:
+ break;
+ case Type.Bytes:
+ break;
+ case Type.String:
+ break;
+ case Type.Record:
+ var recordSchema = (RecordSchema)schema;
+ recordSchema.Fields.SelectMany(f =>
EnumerateSchemasRecursive(f.Schema));
+ break;
+ case Type.Enumeration:
+ break;
+ case Type.Array:
+ var arraySchema = (ArraySchema)schema;
+ EnumerateSchemasRecursive(arraySchema.ItemSchema);
+ break;
+ case Type.Map:
+ var mapSchema = (MapSchema)schema;
+ EnumerateSchemasRecursive(mapSchema.ValueSchema);
+ break;
+ case Type.Union:
+ var unionSchema = (UnionSchema)schema;
+ foreach (var innerSchema in unionSchema.Schemas)
+ {
+ EnumerateSchemasRecursive(innerSchema);
+ }
+
Review comment:
Remove whitespace plz
##########
File path: lang/csharp/src/apache/test/Schema/SchemaTests.cs
##########
@@ -417,6 +595,19 @@ public void TestFixedDoc(string s, string expectedDoc)
Assert.AreEqual(expectedDoc, fs.Documentation);
}
+ [TestCase]
+ public void TestFixedCreation()
+ {
+ string s =
@"{""type"":""fixed"",""name"":""fixedName"",""namespace"":""space"",""aliases"":[""space.fixedOldName""],""size"":10}";
+
+ FixedSchema fixedSchema = FixedSchema.Create("fixedName", 10,
"space", new[] { "fixedOldName" }, null);
+
+ Assert.AreEqual("fixedName", fixedSchema.Name);
+ Assert.AreEqual("space.fixedName", fixedSchema.Fullname);
+ Assert.AreEqual(10, fixedSchema.Size);
+ Assert.AreEqual(s, fixedSchema.ToString());
+ }
+
Review comment:
Should `MapSchema.Create(...)` tested as well?
##########
File path: lang/csharp/src/apache/main/Schema/EnumSchema.cs
##########
@@ -98,20 +128,50 @@ internal static EnumSchema NewInstance(JToken jtok,
PropertyMap props, SchemaNam
/// <param name="names">list of named schema already read</param>
/// <param name="doc">documentation for this named schema</param>
/// <param name="defaultSymbol">default symbol</param>
+ /// <param name="shouldThrowParseException">If true, will throw <see
cref="SchemaParseException"/> in case of an error. If false, will throw <see
cref="AvroException"/> in case of an error.</param>
private EnumSchema(SchemaName name, IList<SchemaName> aliases,
List<string> symbols,
IDictionary<String, int> symbolMap, PropertyMap
props, SchemaNames names,
- string doc, string defaultSymbol)
+ string doc, string defaultSymbol, bool
shouldThrowParseException = true)
: 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 CreateException("name cannot be null
for enum schema.", shouldThrowParseException);
this.Symbols = symbols;
this.symbolMap = symbolMap;
if (null != defaultSymbol && !symbolMap.ContainsKey(defaultSymbol))
- throw new SchemaParseException($"Default symbol:
{defaultSymbol} not found in symbols");
+ throw CreateException($"Default symbol: {defaultSymbol} not
found in symbols", shouldThrowParseException);
Default = defaultSymbol;
}
+ private Exception CreateException(string message, bool
shouldCreateParseException)
+ {
+ return shouldCreateParseException ? new
SchemaParseException(message) : new AvroException(message);
+ }
+
+ /// <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 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 (symbolMap.ContainsKey(symbol))
Review comment:
Are null or empty strings allowed as a symbol? Should it throw
ArgumentException if `string.IsNullOrEmpty(symbol)`?
##########
File path: lang/csharp/src/apache/main/Schema/RecordSchema.cs
##########
@@ -162,13 +281,30 @@ private static Field createField(JToken jfield, int pos,
SchemaNames names, stri
return new Field(schema, name, aliases, pos, doc, defaultValue,
sortorder, props);
}
- private static void addToFieldMap(Dictionary<string, Field> map,
string name, Field field)
+ private static void addParsedFieldToFieldMap(Dictionary<string, Field>
map, string name, Field field)
{
if (map.ContainsKey(name))
throw new SchemaParseException("field or alias " + name + " is
a duplicate name");
map.Add(name, field);
}
+ private static void addToFieldMap(Dictionary<string, Field> map,
string name, Field field)
Review comment:
Is it just a workaround to change the exception type? If yes I would
change the exception to the new type and call it out. This seems to me more
confusing :)
##########
File path: lang/csharp/src/apache/main/Schema/EnumSchema.cs
##########
@@ -47,6 +48,35 @@ public class EnumSchema : NamedSchema
/// </summary>
public int Count { get { return Symbols.Count; } }
+ /// <summary>
+ /// Initializes a new instance of the <see cref="EnumSchema"/> class.
+ /// </summary>
+ /// <param name="name">Name of enum</param>
+ /// <param name="space">Namespace of enum</param>
+ /// <param name="aliases">List of aliases for the name</param>
+ /// <param name="symbols">List of enum symbols</param>
+ /// <param name="customProperties">Custom properties on this
schema</param>
+ /// <param name="doc">Documentation for this named schema</param>
+ /// <param name="defaultSymbol"></param>
+ public static EnumSchema Create(string name,
+ IEnumerable<string> symbols,
+ string space = null,
+ IEnumerable<string> aliases = null,
+ PropertyMap customProperties = null,
+ string doc = null,
+ string defaultSymbol = null)
Review comment:
Should this have an addition parameter at the end? `bool
shouldThrowParseException = false`
##########
File path: lang/csharp/src/apache/main/Schema/RecordSchema.cs
##########
@@ -162,13 +281,30 @@ private static Field createField(JToken jfield, int pos,
SchemaNames names, stri
return new Field(schema, name, aliases, pos, doc, defaultValue,
sortorder, props);
}
- private static void addToFieldMap(Dictionary<string, Field> map,
string name, Field field)
+ private static void addParsedFieldToFieldMap(Dictionary<string, Field>
map, string name, Field field)
{
if (map.ContainsKey(name))
throw new SchemaParseException("field or alias " + name + " is
a duplicate name");
map.Add(name, field);
}
+ private static void addToFieldMap(Dictionary<string, Field> map,
string name, Field field)
Review comment:
Why is addParsedFieldToFieldMap needed? It seems identical to
addToFieldMap. Can we just keep `addToFieldMap` as is ans use it instead of
`addParsedFieldToFieldMap`?
##########
File path: lang/csharp/src/apache/test/Schema/SchemaTests.cs
##########
@@ -335,6 +455,34 @@ public void TestEnumDefaultSymbolDoesntExist(string s)
Assert.Throws<SchemaParseException>(() => Schema.Parse(s));
}
+ [TestCase("name", new string[] { "A", "B" }, "s", new[] { "L1", "L2"
}, "regular enum", null, "name", "s")]
+ [TestCase("s.name", new string[] { "A", "B" }, null, new[] { "L1",
"L2" }, "internal namespace", null, "name", "s")]
+ [TestCase("name", new string[] { "A", "B" }, null, new[] { "L1", "L2"
}, "no namespace", null, "name", null)]
+ [TestCase("name", new string[] { "A", "B" }, null, new[] { "L1", "L2"
}, "with default value", "A", "name", null)]
+ public void TestEnumCreation(string name, string[] symbols, string
space, string[] aliases, string doc, string defaultSymbol, string expectedName,
string expectedNamespace)
+ {
+ EnumSchema enumSchema = EnumSchema.Create(name, symbols, space,
aliases, null, doc, defaultSymbol);
+
+ Assert.AreEqual(expectedName, enumSchema.Name);
+ CollectionAssert.AreEqual(symbols, enumSchema.Symbols);
+ Assert.AreEqual(expectedNamespace, enumSchema.Namespace);
+ Assert.AreEqual(Schema.Type.Enumeration, enumSchema.Tag);
+ Assert.AreEqual(doc, enumSchema.Documentation);
+ Assert.AreEqual(defaultSymbol, enumSchema.Default);
+ }
+
+ [TestCase(new object[] { "A", "A" })]
+ public void TestEnumCreationDuplicateSymbols(params string[] symbols)
+ {
+ Assert.Throws<AvroException>(() => EnumSchema.Create("Name",
symbols));
+ }
+
+ [TestCase]
+ public void TestEnumCreationDefaultSymbolDoesntExists()
+ {
+ Assert.Throws<AvroException>(() => EnumSchema.Create("name", new[]
{ "A", "B" }, defaultSymbol: "C"));
+ }
+
Review comment:
Add unit tests for invalid symbols if there is any. e.g. `null`, `""`,
`" "` ...
--
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]
Issue Time Tracking
-------------------
Worklog Id: (was: 749381)
Time Spent: 2h 10m (was: 2h)
> SchemaBuilder equivalent or other means of schema creation
> ----------------------------------------------------------
>
> Key: AVRO-2211
> URL: https://issues.apache.org/jira/browse/AVRO-2211
> Project: Apache Avro
> Issue Type: Wish
> Components: csharp
> Reporter: Dan Stelljes
> Priority: Major
> Labels: pull-request-available
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> Currently, the only way to create a schema is via the Schema.Parse method.
> We'd like to be able to build a schema programmatically—would it be possible
> to (1) introduce an analogue to the SchemaBuilder from the Java library or
> (2) expose constructors on Schema and its descendants?
> Would be more than happy to contribute work on this given some direction.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
