Add field_id to Column

[acolombi]

I'm interested in adding field_id to Column to make it easier to write Iceberg tables using ParquetSharp for the Parquet bits. I tried to follow the example left in #509, which added field_id to the underlying node. I was a little confused by that PR's change to the PublicAPI.Shipped files, so I approached that differently here, but happy to change approach to whatever makes sense. Thanks!

[adamreeve]

Thanks for making a PR @acolombi! It looks like there are some issues with backwards compatibility that need to be addressed, I've left some suggestions.

[adamreeve]

By the way, you should be able to auto-generate the required changes in PublicAPI.Unshipped.txt with dotnet format analyzers --diagnostics=RS0016.

And there are more details on maintaining backwards compatibility when adding method parameters at https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md

[acolombi]

By the way, you should be able to auto-generate the required changes in PublicAPI.Unshipped.txt with dotnet format analyzers --diagnostics=RS0016.

And there are more details on maintaining backwards compatibility when adding method parameters at https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md

I did try that, and the results I got were not what I expected. I'll include them in this upcoming commit.

[acolombi]

From a source compatibility perspective, this API should be available across two constructors:

• Column(Type logicalSystemType, string name, LogicalType? logicalTypeOverride)
• Column(Type logicalSystemType, string name, LogicalType? logicalTypeOverride = null, int length = -1, int fieldId = -1)

So I think I'm covered.

[adamreeve]

Thanks yes this looks correct to me, I've just pushed a tweak to mark these as removed in PublicAPI.Unshipped.txt rather than remove them from PublicAPI.Shipped.txt. (To be fair this doesn't seem to be documented, but we don't want to remove them from the shipped file until a release is made).

[acolombi]

Alright. This version builds and passes all the tests, and should have API and ABI compatibility. I still feel uncertain about the PublicAPI files, so hopefully I'm not causing too much chaos in my efforts to make those right!

[marcin-krystianc]

LGTM 👍