AVRO-4115: [PHP] Fix long encoding #3303
Conversation
|
@thiagorb @MichelHartmann can you guys take a look at this please? |
|
Looks good to me! |
| */ | ||
| class AvroVarint | ||
| { | ||
| public static function encodeLong(int $n): string |
There was a problem hiding this comment.
IMO it would be good to add some documentation for the new public methods in this class. The old methods (in AvroIOBinaryDecoder have at least @param, @returns, etc.
Also in the other SDKs the unit tests are associated with the class they test.
Here we don't have any tests. There are tests in DatumIOTest but it will be harder/confusing for the next contributor to make the connection.
There was a problem hiding this comment.
@martin-g the PHP DocBlocks are a rather old-fashion thing in PHP, from the time it didn't support type-hinting. I'd suggest dropping them altogether, in favor of proper type-hinting, unless the documentation actually adds some information other than types.
There was a problem hiding this comment.
Anything is better than nothing.
| namespace Apache\Avro; | ||
|
|
||
| /** | ||
| * Varint encoding/decoding for 64bit php |
There was a problem hiding this comment.
Some more information would be nice!
With a link to the specification and/or other resource explaining what this is about.
What is the purpose of the change
This PR fixes PHP long encoding on 64bit platforms as the current implementation does not support longs bigger than
1<<61.Verifying this change
I've added new tests to cover more scenarios and uncommented existing tests.
https://issues.apache.org/jira/browse/AVRO-4115