Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-38348: [C#] Make PrimitiveArray<T> support IReadOnlyList<T?> #38680

Merged
merged 3 commits into from
Nov 19, 2023

Conversation

voidstar69
Copy link
Contributor

@voidstar69 voidstar69 commented Nov 11, 2023

What changes are included in this PR?

Make Arrow arrays of scalar type T implement the same semantic contract as IReadOnlyList<T?>.
Note that this PR does not include similar support for ICollection<T?>. I could add that support in this PR or a future PR.

Are these changes tested?

This PR includes unit tests of the implemented IReadOnlyList<T?> methods.

Copy link

⚠️ GitHub issue #38348 has been automatically assigned in GitHub to PR creator.

@CurtHagenlocher
Copy link
Contributor

I was thinking this would include all scalar arrays, e.g. TimestampArray : IReadOnlyList<DateTime?>, StringArray : IReadOnlyList, TimeXXArray : IReadOnlyList<TimeOnly?>, DateXXArray : IReadOnlyList<DateOnly?>, IReadOnlyList<DateTime?>, BooleanArray : IReadOnlyList<bool?>, BinaryArray : IReadOnlyList<byte[]>.

The original issue also suggests implementing ICollection.

Would you file followup work items so that this one can be marked as closed if checked-in as-is?

@voidstar69
Copy link
Contributor Author

I was thinking this would include all scalar arrays, e.g. TimestampArray : IReadOnlyList<DateTime?>, StringArray : IReadOnlyList, TimeXXArray : IReadOnlyList<TimeOnly?>, DateXXArray : IReadOnlyList<DateOnly?>, IReadOnlyList<DateTime?>, BooleanArray : IReadOnlyList<bool?>, BinaryArray : IReadOnlyList<byte[]>.

The original issue also suggests implementing ICollection.

Would you file followup work items so that this one can be marked as closed if checked-in as-is?

I have implemented this for all the scalar arrays in your example. Are there any more scalar arrays that need to support IReadOnlyList?

For TimestampArray it seemed more natural for it to implement IReadOnlyList<DateTimeOffset?>.

Note that I implemented the various GetEnumerator methods in two distinct ways, a verbose for-loop and a concise LINQ statement. Let me know which you think is better and I will standardise on that form.

I created a followup issue #38692 for adding support for ICollection<T?>.

@CurtHagenlocher
Copy link
Contributor

Note that I implemented the various GetEnumerator methods in two distinct ways, a verbose for-loop and a concise LINQ statement. Let me know which you think is better and I will standardise on that form.

While concision is very nice, a for-loop allows inlining in a way that delegates don't (or don't yet at least) and I think that performance is more important than concision code in infrastructure.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 19, 2023
@CurtHagenlocher CurtHagenlocher merged commit 96e62d8 into apache:main Nov 19, 2023
9 checks passed
@CurtHagenlocher CurtHagenlocher removed the awaiting committer review Awaiting committer review label Nov 19, 2023
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 96e62d8.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@voidstar69 voidstar69 deleted the 38348 branch November 19, 2023 20:40
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…apache#38680)

### What changes are included in this PR?

Make Arrow arrays of scalar type T implement the same semantic contract as IReadOnlyList<T?>.
Note that this PR does not include similar support for ICollection<T?>. I could add that support in this PR or a future PR.

### Are these changes tested?

This PR includes unit tests of the implemented IReadOnlyList<T?> methods.
* Closes: apache#38348

Authored-by: voidstar69 <[email protected]>
Signed-off-by: Curt Hagenlocher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C#] Implement IReadOnlyList<T?> on scalar arrays
2 participants