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-37199: [C++] Expose a span converter for Buffer and ArraySpan #38027

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Oct 5, 2023

Rationale for this change

Convenience. We can have such a helper at the buffer and array data level.

What changes are included in this PR?

Add Buffer::span_as, Buffer::mutuable_span_as and ArraySpan::GetSpan.

Are these changes tested?

No, but I'm happy to add some test if needed.

Are there any user-facing changes?

Yes, new public functions.

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

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

@github-actions github-actions bot added the awaiting review Awaiting review label Oct 5, 2023
@mapleFU
Copy link
Member

mapleFU commented Oct 5, 2023

General looks ok to me, cc @bkietz

@bkietz
Copy link
Member

bkietz commented Oct 5, 2023

I think it'd be better to avoid tying this to the concrete Array classes. The span would expose values masked by null bits which could lead to unfortunate surprises. FWIW these classes already expose begin, end, operator[] yields std::optional for usability.

I think it'd be more useful to expose such a helper at the buffer or array data level, for example as

  • Buffer::span analogous to Buffer::data_as
  • ArraySpan::GetSpan analogous to ArraySpan::GetValues
    (along with equivalents for mutable buffers)

@jsjtxietian
Copy link
Contributor Author

I think it'd be more useful to expose such a helper at the buffer or array data level

You have a valid point and I think that makes a lot of sense.
cc @pitrou what are your thoughts on this as I might have misunderstood the original intent of this issue.

@pitrou
Copy link
Member

pitrou commented Oct 10, 2023

I think it'd be more useful to expose such a helper at the buffer or array data level, for example as

That sounds ok to me.

@jsjtxietian jsjtxietian marked this pull request as draft October 12, 2023 02:11
@jsjtxietian jsjtxietian force-pushed the expose-span-converter-for-primitive-concrete-array branch from 7e69c43 to ccd34ac Compare October 28, 2023 04:01
@jsjtxietian jsjtxietian marked this pull request as ready for review October 28, 2023 04:01
@jsjtxietian jsjtxietian marked this pull request as draft October 28, 2023 04:23
@jsjtxietian jsjtxietian force-pushed the expose-span-converter-for-primitive-concrete-array branch 4 times, most recently from 72dcf01 to 7c50bd4 Compare October 28, 2023 07:29
@jsjtxietian jsjtxietian changed the title GH-37199: [C++] Expose a span converter for primitive concrete arrays GH-37199: [C++] Expose a span converter for Buffer and ArraySpan Oct 28, 2023
cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Oct 30, 2023
@jsjtxietian jsjtxietian force-pushed the expose-span-converter-for-primitive-concrete-array branch from 7c50bd4 to 4fcc934 Compare November 4, 2023 14:45
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 4, 2023
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Nov 8, 2023
@jsjtxietian jsjtxietian marked this pull request as ready for review November 9, 2023 02:11
@jsjtxietian
Copy link
Contributor Author

Sorry for the abuse of CI, my local env was messed up accidentally :(

Copy link
Member

@bkietz bkietz left a comment

Choose a reason for hiding this comment

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

Sorry, one more change:

cpp/src/arrow/array/data.h Show resolved Hide resolved
@github-actions github-actions bot removed the awaiting merge Awaiting merge label Nov 9, 2023
@jsjtxietian jsjtxietian force-pushed the expose-span-converter-for-primitive-concrete-array branch from 4fcc934 to b357cb5 Compare November 11, 2023 06:00
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 11, 2023
cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 13, 2023
@jsjtxietian jsjtxietian marked this pull request as draft November 25, 2023 18:02
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 25, 2023
@jsjtxietian jsjtxietian force-pushed the expose-span-converter-for-primitive-concrete-array branch from 313f86c to d8938ca Compare November 26, 2023 12:49
@jsjtxietian jsjtxietian force-pushed the expose-span-converter-for-primitive-concrete-array branch from d8938ca to d843ba4 Compare November 26, 2023 13:54
cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved
cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved
cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved
cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved
cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved
cpp/src/arrow/array/data.h Show resolved Hide resolved
cpp/src/arrow/array/data.h Outdated Show resolved Hide resolved
@jsjtxietian jsjtxietian marked this pull request as ready for review December 17, 2023 14:51
@jsjtxietian jsjtxietian force-pushed the expose-span-converter-for-primitive-concrete-array branch from dc04c48 to 414e206 Compare December 17, 2023 15:13
@felipecrv felipecrv merged commit 0552217 into apache:main Dec 18, 2023
34 checks passed
@felipecrv felipecrv removed the awaiting change review Awaiting change review label Dec 18, 2023
@jsjtxietian jsjtxietian deleted the expose-span-converter-for-primitive-concrete-array branch December 19, 2023 02:28
Copy link

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

There were no benchmark performance regressions. 🎉

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

clayburn pushed a commit to clayburn/arrow that referenced this pull request Jan 23, 2024
apache#38027)

### Rationale for this change

Convenience. We can have such a helper at the buffer and array data level.

### What changes are included in this PR?

Add `Buffer::span_as`, `Buffer::mutuable_span_as`  and `ArraySpan::GetSpan`.

### Are these changes tested?

No,  but I'm happy to add some test if needed.

### Are there any user-facing changes?

Yes, new public functions.
* Closes: apache#37199

Authored-by: jsjtxietian <[email protected]>
Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
apache#38027)

### Rationale for this change

Convenience. We can have such a helper at the buffer and array data level.

### What changes are included in this PR?

Add `Buffer::span_as`, `Buffer::mutuable_span_as`  and `ArraySpan::GetSpan`.

### Are these changes tested?

No,  but I'm happy to add some test if needed.

### Are there any user-facing changes?

Yes, new public functions.
* Closes: apache#37199

Authored-by: jsjtxietian <[email protected]>
Signed-off-by: Felipe Oliveira Carvalho <[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++] Primitive concrete arrays could expose a span converter
5 participants