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-37359: [C#] Add ToList() to Decimal128Array and Decimal256Array #37383

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

DanTm99
Copy link
Contributor

@DanTm99 DanTm99 commented Aug 25, 2023

Add ToList() methods to Decimal128Array and Decimal256Array.

@DanTm99 DanTm99 requested a review from westonpace as a code owner August 25, 2023 10:13
@github-actions
Copy link

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

@DanTm99
Copy link
Contributor Author

DanTm99 commented Sep 22, 2023

@westonpace

@CurtHagenlocher
Copy link
Contributor

@DanTm99 I'm curious whether you find useful to option to specify includeNulls=false. I recognize that it already exists for PrimitiveArray and think that API consistency is a valuable property, but at the same time I wonder about the kind of scenarios where this would be used.

If Decimal128Array and Decimal256Array were to implement IReadOnlyList<decimal?> and ICollection<decimal?> -- allowing efficient use of LINQ ToList() -- would that meet your use case?

@DanTm99
Copy link
Contributor Author

DanTm99 commented Oct 19, 2023

@CurtHagenlocher yes, the use case I had for this would even be met by Decimal128Array and Decimal256Array just implementing IEnumerable<decimal?>, as all I need to do is use them with LINQ.

The reason I just added these ToList() methods was because it would meet my use case while also making the classes more consistent with PrimitiveArray<T>.

However, implementing IReadOnlyList<decimal?> and ICollection<decimal?> should be done as part of a separate change, as this change was moreso to bring more consistency to this API.

@DanTm99
Copy link
Contributor Author

DanTm99 commented Nov 30, 2023

@westonpace @CurtHagenlocher Conflict resolved. Ready to review.

@CurtHagenlocher CurtHagenlocher merged commit d357d2d into apache:main Dec 4, 2023
9 checks passed
@CurtHagenlocher CurtHagenlocher removed the awaiting review Awaiting review label Dec 4, 2023
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Dec 4, 2023
Copy link

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…ray (apache#37383)

Add `ToList()` methods to `Decimal128Array` and `Decimal256Array`.

* Closes: apache#37359

Authored-by: Danyaal Khan <[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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C#] Add ToList() to Decimal128Array and Decimal256Array
2 participants