-
Notifications
You must be signed in to change notification settings - Fork 1.7k
AVRO-2211: SchemaBuilder equivalent or other means of schema creation #1597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4443265
43f8160
13ac5ae
476a992
fa87fbf
37967f3
e42f18b
05b77ba
825d746
203f1a5
42feece
744a338
a292148
2bf112c
7e76def
55fa5de
2851e5d
fed7a5f
0a98e7a
0e74ea4
c09b58d
3718dc8
07b3a94
3a9d670
a14ce11
615147b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /* | ||
| * 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.Runtime.CompilerServices; | ||
|
|
||
| [assembly: InternalsVisibleTo("Avro.test, PublicKey=00240000048000009400000006020000002400005253413100040000010001001145636d1b96168c2781abfd60478f45d010fe83dd0f318404cbf67252bca8cd827f24648d47ff682f35e60307c05d3cd89f0b063729cf8d2ebe6510b9e7d295dec6707ec91719d859458981f7ca1cbbea79b702b2fb64d1dbf0881887315345b70fa50fcf91b59e6a937c8d23919d409ee2f1f234cc4c8dbf5a29d3d670f3c9")] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
yanivru marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| if (aliases == null) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add brackets? |
||
| { | ||
| return null; | ||
| } | ||
|
|
||
| SchemaName enclosingSchemaName = new SchemaName(enclosingTypeName, enclosingTypeNamespace, null, null); | ||
| return aliases.Select(alias => new SchemaName(alias, enclosingSchemaName.Namespace, null, null)).ToList(); | ||
yanivru marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,8 @@ | |
| */ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Text; | ||
| using System.Linq; | ||
| using System.Text.RegularExpressions; | ||
| using Newtonsoft.Json.Linq; | ||
|
|
||
| namespace Avro | ||
|
|
@@ -30,7 +31,7 @@ public class EnumSchema : NamedSchema | |
| /// <summary> | ||
| /// List of strings representing the enum symbols | ||
| /// </summary> | ||
| public IList<string> Symbols { get; private set; } | ||
| public IList<string> Symbols { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// The default token to use when deserializing an enum when the provided token is not found | ||
|
|
@@ -47,6 +48,34 @@ 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this have an addition parameter at the end?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The public method should have this parameter, it should always throw AvroException (and not ParseException. The internal constructor can be used by this method (public factory method) or by the NewInstance method (the json parsing method). So I add this parameter to it to choose which exception to throw. But if we don't care about backward compatibility of the inner exception (till now there was SchemaParseException inside SchemaParseException, I can change to SchemaParseException the contains AvroException), I can remove this kinda ugly code.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with the change to AvroException (however not the message if possible). I dont think an inner exception of SchemaParse Exception makes too much difference, it can be just a asimple AvroException.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, changed to AvroException (only the inner exception). Not sure how to add the release note :-) |
||
| { | ||
| return new EnumSchema(new SchemaName(name, space, null, doc), | ||
| Aliases.GetSchemaNames(aliases, name, space), | ||
| symbols.ToList(), | ||
| CreateSymbolsMap(symbols), | ||
| customProperties, | ||
| new SchemaNames(), | ||
| doc, | ||
| defaultSymbol); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Static function to return new instance of EnumSchema | ||
| /// </summary> | ||
|
|
@@ -81,7 +110,7 @@ internal static EnumSchema NewInstance(JToken jtok, PropertyMap props, SchemaNam | |
| return new EnumSchema(name, aliases, symbols, symbolMap, props, names, | ||
| JsonHelper.GetOptionalString(jtok, "doc"), JsonHelper.GetOptionalString(jtok, "default")); | ||
| } | ||
| catch (SchemaParseException e) | ||
| catch (AvroException e) | ||
| { | ||
| throw new SchemaParseException($"{e.Message} at '{jtok.Path}'", e); | ||
| } | ||
|
|
@@ -103,15 +132,49 @@ private EnumSchema(SchemaName name, IList<SchemaName> aliases, List<string> symb | |
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Document usage and exception case in method summary, params, and exceptions |
||
| { | ||
| IDictionary<string, int> symbolMap = new Dictionary<string, int>(); | ||
| int i = 0; | ||
| foreach (var symbol in symbols) | ||
| { | ||
| ValidateSymbolName(symbol); | ||
|
|
||
| if (symbolMap.ContainsKey(symbol)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add brackets
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are null or empty strings allowed as a symbol? Should it throw ArgumentException if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ArgumentException or AvroException? The convention everywhere in the code is to throw AvroException even in cases that ArgumentException seems more appropriate.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AvroException makes sense. Is there any other criteria for a symbol name? Maybe add a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, just seen in spec that "Every symbol must match the regular expression [A-Za-z_][A-Za-z0-9_]*" |
||
| { | ||
| throw new AvroException($"Duplicate symbol: {symbol}"); | ||
| } | ||
|
|
||
| symbolMap[symbol] = i++; | ||
| } | ||
|
|
||
| return symbolMap; | ||
| } | ||
|
|
||
| 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}"); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Writes enum schema in JSON format | ||
| /// </summary> | ||
|
|
@@ -127,7 +190,7 @@ protected internal override void WriteJsonFields(Newtonsoft.Json.JsonTextWriter | |
| foreach (string s in this.Symbols) | ||
| writer.WriteValue(s); | ||
| writer.WriteEndArray(); | ||
| if (null != Default) | ||
| if (null != Default) | ||
| { | ||
| writer.WritePropertyName("default"); | ||
| writer.WriteValue(Default); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,7 +103,39 @@ public enum SortOrder | |
| /// <summary> | ||
| /// Static comparer object for JSON objects such as the fields default value | ||
| /// </summary> | ||
| internal static JTokenEqualityComparer JtokenEqual = new JTokenEqualityComparer(); | ||
| internal readonly static JTokenEqualityComparer JtokenEqual = new JTokenEqualityComparer(); | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="Field"/> class. | ||
| /// </summary> | ||
| /// <param name="schema">schema for the field type.</param> | ||
| /// <param name="name">name of the field.</param> | ||
| /// <param name="aliases">list of aliases for the name of the field.</param> | ||
| /// <param name="pos">position of the field.</param> | ||
| /// <param name="doc">documentation for the field.</param> | ||
| /// <param name="defaultValue">field's default value if it exists.</param> | ||
| /// <param name="sortorder">sort order of the field.</param> | ||
| /// <param name="customProperties">dictionary that provides access to custom properties.</param> | ||
| public Field(Schema schema, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason you didn't modify the existing constructor?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to add default values to non mandatory parameters. I had to change their order (switch between aliases and pos), and didn't want to make a breaking change. |
||
| string name, | ||
| int pos, | ||
| IList<string> aliases = null, | ||
| string doc = null, | ||
| JToken defaultValue = null, | ||
| SortOrder sortorder = SortOrder.ignore, | ||
| PropertyMap customProperties = null) | ||
| : this(schema, name, aliases, pos, doc, defaultValue, sortorder, customProperties) | ||
| { | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates a new field based on the specified field, with a different position. | ||
| /// </summary> | ||
| /// <returns>A clone of this field with new position.</returns> | ||
| internal Field ChangePosition(int newPosition) | ||
| { | ||
| return new Field(Schema, Name, newPosition, Aliases, Documentation, DefaultValue, Ordering ?? SortOrder.ignore, Props); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// A flag to indicate if reader schema has a field that is missing from writer schema and has a default value | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove and sort usings. Assuming you are using VS.