Fix an incorrect string comparison in avro-python3 leading to corrupted files

[zah]

The corrected code was trying to compare the bytes value returned by GetMeta against a string.
The if was never taken and this was leading to corrupted files when the snappy codes is used.

Here is a minimal test case that demonstrates the issue:

import avro.schema
from avro.datafile import DataFileReader, DataFileWriter
from avro.io import DatumReader, DatumWriter

schema_text = """
{

  "namespace": "example.avro",
  "type": "record",
  "name": "User",
  "fields": [
      {"name": "name", "type": "string"},
      {"name": "favorite_number",  "type": ["int", "null"]},
      {"name": "favorite_color", "type": ["string", "null"]}
  ]
 }
"""

schema = avro.schema.Parse(schema_text)

writer = DataFileWriter(open("users.avro", "wb"), DatumWriter(), schema, codec="snappy")

writer.append({"name": "Alyssa", "favorite_number": 256})
writer.append({"name": "Ben", "favorite_number": 7, "favorite_color": "red"})
writer.close()

reader = DataFileReader(open("users.avro", "rb"), DatumReader())
for user in reader:
    print(user)

reader.close()

[harshach]

+1

[johnsgill3]

Looks good. The codec is set after ensuring UTF8 decoding. Can you take the case provided and turn it into a py-test case for ./build.sh test unit validation?

[spacharya]

Is there a JIRA with this issue? If not can you create one?

[zah]

@spacharya, there is:
https://issues.apache.org/jira/browse/AVRO-2027

Can you comment if there is a new release planned soon? I hope this simple fix will be able to make it in the release.

[spacharya]

• We just had a 1.8.2 release. We intend to have a regular cadence of releases going forward.
• like @johnsgill3 can you convert this into a test? This will help prevent others from breaking this code in future.

Once you add the test, I will merge it in. Do you intend to have this in a 1.8 release or 1.9 release?

[busbey]

We should get this into any release line impacted by the bug.

@zah, if you don't have time to make a test we can file a follow on JIRA to get it added later.

[zah]

Sorry, I was planning to study how test cases are added to the suite, but my work schedule got busier and I had to move on to other things. My initial comment actually provides a minimal test case, so anyone who is more familiar with the test suite can easily add it.

Thanks a lot for your extra efforts to merge this into all release lines.

[kojiromike]

If you look at lines 263 and 286, you can tell that this is a correct change. Tests are nice, but this one is pretty self-evident. I'd like to see this get merged.

However, since this one has been languishing, someone will need to kick off tests on it that have been put in place since it was written. @Fokko is there a way to do that without having to ask the original maintainer to do a rebase on a very old, one-line-change PR?

[Fokko]

@kojiromike Unfortunately is no easy way to do that.

I've added sixsentsinc as a remote, checked out the branch, and rebased onto latest master. Let's see if the tests pass: https://travis-ci.org/Fokko/avro/builds/530698025 If so, I suggest merging this.

[kojiromike]

👏 It passed @Fokko :)

[Fokko]

Awesome!