search

G-Research / ParquetSharp

ParquetSharp is a .NET library for reading and writing Apache Parquet files.
Apache License 2.0
183 stars 49 forks source link

Fix breaking change to LogicalTypeEnum.None value #444

Closed adamreeve closed 6 months ago

adamreeve commented 6 months ago

Fixes #443

adamreeve commented 6 months ago

PS: What an odd decision to put the None as the lat value in the enum. It guarantees that each time a new value is added, the value of None needs to be changed as well.

Yeah, this is a little unfortunate and if we'd foreseen this problem we could maybe have mapped None to something different to what the C++ library uses that wasn't going to clash with a future value.

Should we implement additional measures to prevent such mistakes in the future?

This is a good idea, we have some checks that detect if the C++ enum values change (https://github.com/G-Research/ParquetSharp/blob/9fba13153c272627db4438d064ce829198cda752/cpp/Enums.cpp#L40) but I only thought that meant the C# enums needed updating, I missed that this would break the ABI. Maybe we could have some automated tests that build tests against older versions of ParquetSharp and then run them with the current build, which should detect a range of different ABI breaking scenarios.