search

hamba / avro

A fast Go Avro codec
MIT License
345 stars 77 forks source link

SchemaCompatibility.Compatible has one differ from apache/avro java implementation #403

Closed timmyb32r closed 2 weeks ago

timmyb32r commented 2 weeks ago

Java implementation of apache/avro has a lot of tests on compatibility function, and hamba/avro differs with only one test (other tests are passed). The problem case is - when reader_schema & writer_schema differs by only namespace.

links:

https://github.com/apache/avro/blob/main/lang/java/avro/src/test/java/org/apache/avro/TestSchemas.java#L119

    static final Schema WITHOUT_NS = Schema.createRecord("Record", null, null, false);
    static final Schema WITH_NS = Schema.createRecord("ns.Record", null, null, false);

https://github.com/apache/avro/blob/main/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java#L341

    public static final List<ReaderWriter> COMPATIBLE_READER_WRITER_TEST_CASES = list(
        //...
        new ReaderWriter(WITHOUT_NS, WITH_NS)

https://github.com/apache/avro/blob/main/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java#L389 - test

Where the difference is

into java implementation there are comparison with GetName(): https://github.com/apache/avro/blob/main/lang/java/avro/src/test/java/org/apache/avro/TestSchemaCompatibility.java#L389

  public static boolean schemaNameEquals(final Schema reader, final Schema writer) {
    if (objectsEqual(reader.getName(), writer.getName())) { // <-------------------------------- here compare by name
      return true;
    }
    // Apply reader aliases:
    return reader.getAliases().contains(writer.getFullName());
  }

into hamba/avro there are comparison with GetFullName(): https://github.com/hamba/avro/blob/main/schema_compatibility.go#L182

func (c *SchemaCompatibility) checkSchemaName(reader, writer NamedSchema) error {
    if reader.FullName() != writer.FullName() { // <-------------------------------- here compare by FULL name
        if c.contains(reader.Aliases(), writer.FullName()) {
            return nil
        }
        return fmt.Errorf("reader schema %s and writer schema %s  names do not match", reader.FullName(), writer.FullName())
    }

    return nil
}

Let's make these checks equal!

nrwiersma commented 2 weeks ago

Hi. Thanks for bringing this up. You can confirm all checks pass with this one change?

Out of interest, how are you running the compatibility tests?

timmyb32r commented 2 weeks ago

Out of interest, how are you running the compatibility tests?

I've took java apache/avro compatibility tests, made from them testcases for golang, and run them. Some part was easy to rewrite by hands, some part by regexps. Of course, I couldn't make full-fledged transpilation - i've had only several days, so hand-rewrite + regexps: made the deal.

Actually, I have now golang tool who take TestSchemaCompatibility.java & TestSchemas.java - and generates golang tests from them. But result code look ugly, so I didn't think somebody will be interested in that (I've just put it into my CI for myself). But if you are interested in that tests generator - I can share it.

You can confirm all checks pass with this one change?

yeah, if this check improve - then from java project apache/avro passed next tests: