Python: Add more tests for the Avro writer

[Fokko]

Based on the feedback of Ryan on #7873! Thanks, this actually also uncovered some bugs in the read path.

cc @maxdebayser

[rdblue]

I don't think this should produce a date. Our internal representation for date/time objects are:

• int days from epoch for date
• int (long) micros from epoch for timestamp and timestamptz
• int micros from midnight for time

We do not want to use native date/time representations internally because they require a lot of extra logic to compare and work with in other ways.

[Fokko]

Yes, I reverted this, and also fixed this for the datetime, that would actually break. I've also added some integration tests to cover this.

[rdblue]

Here as well, the encoders and decoders should not use datetime objects.

[Fokko]

This is correct, right? The encoders only work with physical types. The write_date_int accepted a date, and now we do the conversion in the writer itself.

[rdblue]

It's fine to have a writer that work with date, but right now we need one that works with int because that's our internal representation. If someone passes a filter that is d < DATE '2023-08-01' for example, we'll receive that as something like this: LessThan(Ref("d"), IntLiteral(1234)). We want to compare integers rather than dates because that's faster and more reliable. When we read/write Avro, we want to go to that internal representation.

[rdblue]

We also want to be able to read time-millis right? We have readers for it in Java where we just multiple by 1_000 when we get the value.

[Fokko]

This isn't part of the Iceberg spec, I'd rather leave it out unless you have strong concerns here.

[rdblue]

I'm fine either way. Eventually, we'll need to read files that were written for Hive and imported into an Iceberg table, so we are generally more permissive with reads. Since this is mostly about Iceberg metadata right now, we don't need to worry about it.

[rdblue]

Why move this here? The encoder still has decimal logic. I don't mind either way whether decimal is handled by the encoder/decoder or in the reader/writer, but it seems like we should be consistent.

Also, this reader assumes that the decimal is stored as variable-length binary and not fixed (because it is reading the length). The spec requires that decimals are written in Avro as fixed, length minBytesRequired(P): https://iceberg.apache.org/spec/#avro

[Fokko]

Why move this here? The encoder still has decimal logic. I don't mind either way whether decimal is handled by the encoder/decoder or in the reader/writer, but it seems like we should be consistent.

I've moved it to the decimal.py, and created a function bytes_to_decimal, since we already have decimal_to_bytes.

Also, this reader assumes that the decimal is stored as variable-length binary and not fixed (because it is reading the length). The spec requires that decimals are written in Avro as fixed, length minBytesRequired(P): https://iceberg.apache.org/spec/#avro

😱 Ah, this was always wrong, thanks for catching this. Let me add a test there

[rdblue]

decimal_required_bytes(self.precision) should be done in the constructor and reused, right? Or does a frozen dataclass make that hard?

[Fokko]

The frozen class makes it a bit awkward indeed, but did it anyway

[rdblue]

Like the comment above, we don't need to calculate the size every time since this is in a tight loop.

[rdblue]

I think I prefer the way Java does this because it would be a couple of list comprehensions rather than computing the max precision for each byte length for each call.

MAX_PRECISION = tuple( math.floor(math.log10(math.fabs(math.pow(2, 8 * l - 1) - 1))) for l in range(24) )
REQUIRED_LENGTH = tuple( next(l for l in range(24) if p <= MAX_PRECISION[l]) for p in range(40) )

def required_bytes(precision: int) -> int:
    if precision <= 0 or precision >= len(REQUIRED_LENGTH):
        raise ValueError(...)
    return REQUIRED_LENGTH[precision]

[Fokko]

It is cached, and likely that you would call it with the same argument many time, therefore the @lru_cache

[Fokko]

I also like the tuples very much, I'll go with those

[rdblue]

We should technically only go to 38 because that's the max precision for 16 bytes. I'm not sure why the Java code went to 40. That's one more entry than needed in the REQUIRED_BYTES array.

[rdblue]

Overall looks good. Using @lru_cache vs storing the length on the reader/writer is minor, as is the way the required length is calculated.

[Fokko]

Thanks again @rdblue for the comprehensive review, appreciate it! 🙏🏻

[rustyconover]

Hi Friends,

Tonight I'm trying to rebase my Avro reader branch and this PR really wasn't easy to follow.

The PR change description didn't really describe what was decided to be changed without reading all of the comments from the review.

In the future can you please split things like this up with clearer logic written in the change message especially decisions around architecture (such as don't use native date/datetime types). It took me a while to understand why tests were removed.

I'd like to see the commits/PRs say things like:

"Removing use of native datetime types in the Avro reader path, removing relevant tests."

Talking about fixing bugs in the read path just didn't give the justifications why the changes were made.

[Fokko]

Sorry @rustyconover for the confusion here. The PR grew a bit bigger than originally intended, but I can see how it can be confusing. The decoder/encoded should only handle primitive types, and therefore I removed them, but it shouldn't have happened under an ambiguous PR like this one.