Avro1363 C# union schema can now contain multiple entries with the same name and different namespace

[Simon24601]

When processing a UnionSchema, we store a list of all the names that have already been processed. In Java, the fully-resolved names are used (including namespaces), while in C# only the simple name was used. This means that a UnionSchema cannot contain multiple entries with the same name and different namespaces.

I've fixed this by making the code more like the Java, i.e. adding a Fullname method to the base Schema class, and using this when processing the UnionSchema.

I have not checked for the same issue in other languages.

[Simon24601]

Is there anyone in the Avro C# community who can review (and accept) this pull request?

[rdblue]

This looks reasonable to me. @jcustenborder, what do you think?

[jcustenborder]

@rdblue I'll look at it. I only see one nit.

[jcustenborder]

Why the override?

[Simon24601]

@jcustenborder: I've added a virtual Fullname method in the base Schema.cs
class, and wanted to be explicit that it is being overridden to provide
different behaviour. Schema doesn't really have a namespace and can't
distinguish between a Fullname and a Name, while NamedSchema can
distinguish these, so the implementation is richer.

In the UnionSchema parser that I was trying to fix, I needed to make use of
the Fullname to make sure different namespaces are properly separated.
Since we just have a base class Schema in the parser, I needed a virtual
function to make sure the appropriate code was used.

On Fri, Nov 4, 2016 at 8:19 PM, Jeremy Custenborder <
[email protected]> wrote:

@jcustenborder commented on this pull request.

@rdblue https://github.com/rdblue I'll look at it. I only see one nit.

In lang/csharp/src/apache/main/Schema/NamedSchema.cs
#131 (review):

@@ -52,7 +52,7 @@ public string Namespace
///

     /// Namespace.Name of the schema
     /// </summary>

•    public string Fullname
  

•    public override string Fullname
  

Why the override?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#131 (review), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AIUPmo7FVyZ0QpGHwa6e-P0uesmu7AAnks5q65NSgaJpZM4KHGnD
.

[rdblue]

This doesn't apply cleanly to master (I checked out and tried to rebase). @Simon24601, could you please rebase on master and squash these commits? Thanks!

@jcustenborder, does this look about ready to go in? It sounds like your concern was addressed.

[jcustenborder]

@rdblue Looks good to me.