Update to .NET 10.0 SDK #600
Conversation
| <ItemGroup> | ||
| <PackageReference Include="FsUnit" Version="5.1.0" /> | ||
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.9.4" /> | ||
| <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.7.2" /> |
There was a problem hiding this comment.
This update was required because it had a transient dependency to a vulnerable version of Newtonsoft.Json
csharp/ColumnReader.cs
Outdated
|
|
||
| public unsafe long ReadBatch(long batchSize, Span<short> defLevels, Span<short> repLevels, Span<TValue> values, out long valuesRead) | ||
| { | ||
| #pragma warning disable CA2265 |
There was a problem hiding this comment.
A new default warning was introduced in .NET 10. It is disabled because we truly need to compare to null.
There was a problem hiding this comment.
Should we not just be checking if the Spans are empty? The Span can't actually be null as it is a struct so this does look wrong to me.
There was a problem hiding this comment.
Yeah, it should be converted to span.IsEmpty()(https://learn.microsoft.com/en-gb/dotnet/fundamentals/code-analysis/quality-rules/ca2265#example)
There was a problem hiding this comment.
I was 100% sure I had tried to convert them to span.IsEmpty (it's a member, not a method) and that the tests were failing, but I must have made a different mistake back then because it's working just fine now. I'll update the PR soon!
There was a problem hiding this comment.
ok, PR updated - the tests were failing because I had naively replace span == null with span.IsEmpty, but there are places where we passed an empty span for the values that then started to fail (specifically in ParquetSharp.LogicalBatchWriter.ArrayWriter.WriteBatch). I have removed these unnecessary null comparisons as they were not doing what we expected them to do. Please can you both double check my approach?
There was a problem hiding this comment.
The PublicAPIAnalyzers in .NET 10.0 required me to move those symbols into the individual framework-dependent PublicAPI.Shipped.txt files. I am not entirely sure why, but I couldn't make the project build without these changes.
There was a problem hiding this comment.
Looks like it affected anything involving System.IntPtr. Weird.
csharp/ColumnReader.cs
Outdated
|
|
||
| public unsafe long ReadBatch(long batchSize, Span<short> defLevels, Span<short> repLevels, Span<TValue> values, out long valuesRead) | ||
| { | ||
| #pragma warning disable CA2265 |
There was a problem hiding this comment.
Should we not just be checking if the Spans are empty? The Span can't actually be null as it is a struct so this does look wrong to me.
There was a problem hiding this comment.
Looks like it affected anything involving System.IntPtr. Weird.
Targetting net8.0 is enough for net9.0 and net10.0 consumers
|
|
||
| public unsafe long ReadBatch(long batchSize, Span<short> defLevels, Span<short> repLevels, Span<TValue> values, out long valuesRead) | ||
| { | ||
| if (values == null) throw new ArgumentNullException(nameof(values)); |
| /// than the length of <paramref name="defLevels"/> or <paramref name="repLevels"/>.</exception> | ||
| public unsafe void WriteBatch(int numValues, ReadOnlySpan<short> defLevels, ReadOnlySpan<short> repLevels, ReadOnlySpan<TValue> values) | ||
| { | ||
| if (values == null) throw new ArgumentNullException(nameof(values)); |
| if (values == null) throw new ArgumentNullException(nameof(values)); | ||
| if (defLevels == null) throw new ArgumentNullException(nameof(defLevels)); | ||
| if (repLevels == null) throw new ArgumentNullException(nameof(repLevels)); | ||
| if (validBits == null) throw new ArgumentNullException(nameof(validBits)); |
3 participants
This PR drops support for .NET 6.0 (support from Microsoft ended more than a year ago, on Nov 12th 2024) and adds support for .NET 10.0. It also requires a .NET 10.0 SDK to build.