GH-41047: [C#] Address performance issue of reading from StringArray #41048
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or In the case of PARQUET issues on JIRA the title also supports: See also: |
CurtHagenlocher
changed the title
|
|
CurtHagenlocher
requested changes
Apr 7, 2024
github-actions
bot
added
awaiting committer review
CurtHagenlocher
approved these changes
Apr 8, 2024
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit fe38d47. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
raulcd
pushed a commit
that referenced
this pull request
Apr 9, 2024
…41048) ### Rationale for this change The motivation here is to address #41047. There is severe performance drawback in reading a StringArray as value array of a DictionaryArray, because of repeated and unnecessary UTF 8 string decoding. ### What changes are included in this PR? - Added a new function Materialize() to materialize the values to a list. When materialized, GetString() reads from the vector directly. - Added test coverage. ### Are these changes tested? Yes ### Are there any user-facing changes? No. This change maintains backwards compatibility on the API surface. It is up to the client application to decide whether to materialize the array and gain performance. * GitHub Issue: #41047 Authored-by: Keshuang Shen <keshen@microsoft.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
verma-kartik
pushed a commit
to verma-kartik/arrow
that referenced
this pull request
Apr 11, 2024
…Array (apache#41048) ### Rationale for this change The motivation here is to address apache#41047. There is severe performance drawback in reading a StringArray as value array of a DictionaryArray, because of repeated and unnecessary UTF 8 string decoding. ### What changes are included in this PR? - Added a new function Materialize() to materialize the values to a list. When materialized, GetString() reads from the vector directly. - Added test coverage. ### Are these changes tested? Yes ### Are there any user-facing changes? No. This change maintains backwards compatibility on the API surface. It is up to the client application to decide whether to materialize the array and gain performance. * GitHub Issue: apache#41047 Authored-by: Keshuang Shen <keshen@microsoft.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
vibhatha
pushed a commit
to vibhatha/arrow
that referenced
this pull request
Apr 15, 2024
…Array (apache#41048) ### Rationale for this change The motivation here is to address apache#41047. There is severe performance drawback in reading a StringArray as value array of a DictionaryArray, because of repeated and unnecessary UTF 8 string decoding. ### What changes are included in this PR? - Added a new function Materialize() to materialize the values to a list. When materialized, GetString() reads from the vector directly. - Added test coverage. ### Are these changes tested? Yes ### Are there any user-facing changes? No. This change maintains backwards compatibility on the API surface. It is up to the client application to decide whether to materialize the array and gain performance. * GitHub Issue: apache#41047 Authored-by: Keshuang Shen <keshen@microsoft.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
tolleybot
pushed a commit
to tmct/arrow
that referenced
this pull request
May 2, 2024
…Array (apache#41048) ### Rationale for this change The motivation here is to address apache#41047. There is severe performance drawback in reading a StringArray as value array of a DictionaryArray, because of repeated and unnecessary UTF 8 string decoding. ### What changes are included in this PR? - Added a new function Materialize() to materialize the values to a list. When materialized, GetString() reads from the vector directly. - Added test coverage. ### Are these changes tested? Yes ### Are there any user-facing changes? No. This change maintains backwards compatibility on the API surface. It is up to the client application to decide whether to materialize the array and gain performance. * GitHub Issue: apache#41047 Authored-by: Keshuang Shen <keshen@microsoft.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
vibhatha
pushed a commit
to vibhatha/arrow
that referenced
this pull request
May 25, 2024
…Array (apache#41048) ### Rationale for this change The motivation here is to address apache#41047. There is severe performance drawback in reading a StringArray as value array of a DictionaryArray, because of repeated and unnecessary UTF 8 string decoding. ### What changes are included in this PR? - Added a new function Materialize() to materialize the values to a list. When materialized, GetString() reads from the vector directly. - Added test coverage. ### Are these changes tested? Yes ### Are there any user-facing changes? No. This change maintains backwards compatibility on the API surface. It is up to the client application to decide whether to materialize the array and gain performance. * GitHub Issue: apache#41047 Authored-by: Keshuang Shen <keshen@microsoft.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
CurtHagenlocher
pushed a commit
that referenced
this pull request
Jul 19, 2024
…#43269) ### Rationale for this change See #43266. Note that LargeBinary and LargeString are still limited to 2 GiB buffers, and LargeList is limited to offsets that can be represented as int32. ### What changes are included in this PR? * Add new Array subtypes: LargeBinaryArray, LargeStringArray and LargeListArray * Support round-tripping these array types via the IPC format * Support round-tripping these array types via the C Data Interface * Improve error messages when importing arrays that are too large via IPC or C Data Interface * Enable integration tests for the new types * Update documentation ### Are these changes tested? Yes, I've added some basic tests specifically for the new array types, and added these to the test data generator so they're covered by the existing tests for round tripping using IPC and C Data Interface. ### Are there any user-facing changes? Yes, this is a new user facing feature. ### Implementation notes * I haven't added builders for these new array types. Given they're added to help with interoperability with other libraries, I wouldn't expect .NET users to build arrays of these types as they provide no other benefit over the non-large types until we have proper large memory support. But I'm happy to add this if it would be useful. * The new array types share a lot of logic with the non-large types. I considered trying to consolidate this logic by adding a new `BinaryArrayBase<TOffset>` class for example, but I think this would require generic math support to work nicely, and would still complicate the code quite a bit and add extra virtual method call overhead. So I think it's fine to keep these new Array subtypes independent from the non-large types. * I haven't included support for materializing a LargeStringArray (see #41048). I'm not sure whether there would be a use for this, but it could be added later if needed. * GitHub Issue: #43266 Authored-by: Adam Reeve <adreeve@gmail.com> Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
The motivation here is to address #41047. There is severe performance drawback in reading a StringArray as value array of a DictionaryArray, because of repeated and unnecessary UTF 8 string decoding.
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No. This change maintains backwards compatibility on the API surface. It is up to the client application to decide whether to materialize the array and gain performance.