Skip to content

GH-35627: [Format][Integration] Add string-view to arrow format - #37526

Merged
bkietz merged 1 commit into
apache:mainfrom
bkietz:35627-string-view-format-only
Sep 21, 2023
Merged

GH-35627: [Format][Integration] Add string-view to arrow format#37526
bkietz merged 1 commit into
apache:mainfrom
bkietz:35627-string-view-format-only

Conversation

@bkietz

@bkietzbkietz commented Sep 1, 2023

Copy link
Copy Markdown
Member

String view (and equivalent non-utf8 binary view) is an alternative representation for
variable length strings which offers greater efficiency for several common operations.
This representation is in use by UmbraDB, DuckDB, and Velox. Where those databases use
a raw pointer to out-of-line strings this PR uses a pair of 32 bit integers as a
buffer index and offset, which

  • makes explicit the guarantee that lifetime of all character data is equal
    to that of the array which views it, which is critical for confident
    consumption across an interface boundary
  • makes the arrays meaningfully serializable and
    venue agnostic; directly usable in shared memory without modification
  • allows easy validation

This PR is extracted from #35628 to unblock independent PRs now that the vote has passed, including:

  • New types added to Schema.fbs
  • Message.fbs amended to support variable buffer counts between string view chunks
  • datagen.py extended to produce integration JSON for string view arrays
  • Columnar.rst amended with a description of the string view format

@zeroshadezeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 5, 2023
Comment threaddocs/source/format/Columnar.rst Outdated
Comment threaddocs/source/format/Columnar.rst Outdated
Comment threaddocs/source/format/Columnar.rst Outdated
Comment threaddocs/source/format/Columnar.rst Outdated
Comment threaddocs/source/format/Columnar.rst Outdated
Comment threaddocs/source/format/Columnar.rst Outdated
Comment threaddocs/source/format/Columnar.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let's please not make a special case for binary-view here. We use signed offsets and lengths everywhere else.

Suggested change
by a raw pointer. All integers (length, buffer index, and offset) are unsigned
by a raw pointer. All integers (length, buffer index, and offset) are signed

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should discuss this on the ML, I'll raise this there

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the standard seems to have adopted signed integers for sizes in every other case. It seems very strange to impose that only for new types?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a requirement to be imposed on sizes in all cases going forward. It's only relevant to Utf8View because existing implementations of string view already use unsigned integers here, so the concern is whether to keep compatibility with those or maintain the convention of the arrow format. If you'd like to +1 signed integers, please say so on the ML!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the mailing list, I've updated this to signed integers.

Comment threaddocs/source/format/Columnar.rst Outdated
Comment threadformat/Schema.fbs Outdated
Comment threadformat/Message.fbs Outdated
Comment threaddocs/source/format/Columnar.rst Outdated
Comment threadformat/Schema.fbs Outdated
Comment threadformat/Schema.fbs Outdated
Comment threadformat/Schema.fbs Outdated
Comment threaddev/archery/archery/integration/datagen.py Outdated
Comment threaddev/archery/archery/integration/datagen.py Outdated
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Sep 18, 2023

@mapleFUmapleFU left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@bkietz
bkietzforce-pushed the 35627-string-view-format-only branch from 933b5d6 to 7084f71CompareSeptember 19, 2023 17:39
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Sep 19, 2023
@bkietz
bkietzforce-pushed the 35627-string-view-format-only branch from 7084f71 to b01b801CompareSeptember 20, 2023 15:14
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 20, 2023
@bkietz

Copy link
Copy Markdown
MemberAuthor

CI failures 1, 2 are unrelated, see #37803

@bkietz
bkietzforce-pushed the 35627-string-view-format-only branch from b01b801 to 4a24b36CompareSeptember 21, 2023 00:10
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 21, 2023
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Sep 21, 2023
@bkietz

Copy link
Copy Markdown
MemberAuthor

+1, thanks all!

@bkietz
bkietz merged commit 9d6d501 into apache:mainSep 21, 2023
@bkietzbkietz removed the awaiting merge Awaiting merge label Sep 21, 2023
@bkietz
bkietz deleted the 35627-string-view-format-only branch September 21, 2023 12:01
@alamb

Copy link
Copy Markdown
Contributor

🎉

@conbench-apache-arrow

Copy link
Copy Markdown

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

There were 4 benchmark results indicating a performance regression:

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

bkietz added a commit that referenced this pull request Oct 26, 2023
### Rationale for this change
After the PR changing the spec and schema ( #37526 ) is accepted, this PR will be undrafted. It adds the minimal addition of a C++ implementation and was extracted from the original C++ Utf8View pr ( #35628 ) for ease of review.
### What changes are included in this PR?
- The new types are available with new subclasses of DataType, Array, ArrayBuilder, ...
- The values of string view arrays can be visited as `std::string_view` as with StringArray
- String view arrays can be round tripped through IPC and integration JSON
* Closes: #37710
Relevant mailing list discussions: * https://lists.apache.org/thread/l8t1vj5x1wdf75mdw3wfjvnxrfy5xomy
* https://lists.apache.org/thread/3qhkomvvc69v3gkotbwldyko7yk9cs9k
Authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Signed-off-by: Benjamin Kietzman <bengilgit@gmail.com>
Sign up for freeto 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.

[Format] Add string view type to the arrow format

9 participants

@bkietz@alamb@wesm@zeroshade@pitrou@tustvold@wjones127@paleolimbot@mapleFU