GH-46128:[C++] Add CompactArray method to BinaryViewArray types

[andishgar]

Rationale for this change

As we discussed here, the following method copies only data that is used in data buffers to a new buffer

What changes are included in this PR?

Add a new method the name of which is CompactArray

Are these changes tested?

I run the relevant unit tests

Are there any user-facing changes?

Yes, I add ComapctArray method to BinaryViewArray class

• GitHub Issue: [C++][Compute] Reducing memory usage when casting from other String types to StringView #46128

[github-actions]

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

[andishgar]

@pitrou When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do? Should I drop this PR?

[andishgar]

Another note:
I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

#include<iostream>
#include<arrow/api.h>
arrow::Status Run() {
int32_t length = std::numeric_limits<int32_t>::max();
std::string input( length,'a');
arrow::StringViewBuilder builder;
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_ASSIGN_OR_RAISE(auto result, builder.Finish());
arrow::ArraySpan span(*result->data());
// The following line ends to Capacity errorARROW_RETURN_NOT_OK(builder.AppendArraySlice(span,0,2));
returnarrow::Status::OK();
}
intmain() {
auto status = Run();
if (!status.ok()) {
std::cerr << status.ToString() << std::endl;
}
return0;
}

Error

Capacity error: BinaryView or StringView elements cannot reference strings larger than 2GB

[pitrou]

When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do?

I still think it's a useful utility method to have. cc @bkietz for additional opinions.

I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

[andishgar]

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

Thank you for your response. Yes, of course

[pitrou]

Thanks for posting this PR and sorry for the delay @andishgar . I added some comments below.

[andishgar]

@pitrou Thanks for the review. I’ll look into it and follow up soon.

[andishgar]

@pitrou
Note: This is unrelated to the PR.
I plan to contribute to Arrow until August 2, and then I will resume contributions from August 30.
If you think there are any issues or pull requests related to me that have higher priority and should be addressed before August 2, please let me know so I can take care of them in time.

Another Note
If you plan to review any of my other PRs, I’d really appreciate it if you could let me know beforehand. I’d like to take a final look and possibly make some improvements first.
(I’ve had a few valuable code review sessions with Kou and learned quite a bit through them. Since most of my code was written before those PRs , I’d prefer to apply those learnings before others take the time to review my work.)

[andishgar]

@pitrou I’ve applied your suggestion. The changes are ready for review.

[andishgar]

Thank you for your feedback. I will look into this.

[andishgar]

@pitrou I applied your suggestions.
Two notes:

1-Regarding this comment, my current algorithm for adding intervals even updates interval.start, so I believe the suggestion to use a std::map doesn't apply in this case. However, if you're still in favor of using std::map, please let me know, and I’ll adjust the logic accordingly.

2- I use a lot of if statements in TryFastInsert for MergeOrInsertInterval, and the tests I wrote cover all the branches. However, if you believe the number of if statements should be reduced, it is possible, since most of them are independent of each other. That said, it's also possible to increase the number of if statements—for example, in every place I use std::max, it could be broken down into several ifs. Another example is the "less than" case in TryFastInsert, which I mentioned in my comments.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

[andishgar]

Rationale for this change

As we discussed here, the following method copies only data that is used in data buffers to a new buffer

What changes are included in this PR?

Add a new method the name of which is CompactArray

Are these changes tested?

I run the relevant unit tests

Are there any user-facing changes?

Yes, I add ComapctArray method to BinaryViewArray class

• GitHub Issue: [C++][Compute] Reducing memory usage when casting from other String types to StringView #46128

[github-actions]

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

[andishgar]

@pitrou When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do? Should I drop this PR?

[andishgar]

Another note:
I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

#include<iostream>
#include<arrow/api.h>
arrow::Status Run() {
int32_t length = std::numeric_limits<int32_t>::max();
std::string input( length,'a');
arrow::StringViewBuilder builder;
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_ASSIGN_OR_RAISE(auto result, builder.Finish());
arrow::ArraySpan span(*result->data());
// The following line ends to Capacity errorARROW_RETURN_NOT_OK(builder.AppendArraySlice(span,0,2));
returnarrow::Status::OK();
}
intmain() {
auto status = Run();
if (!status.ok()) {
std::cerr << status.ToString() << std::endl;
}
return0;
}

Error

Capacity error: BinaryView or StringView elements cannot reference strings larger than 2GB

[pitrou]

When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do?

I still think it's a useful utility method to have. cc @bkietz for additional opinions.

I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

[andishgar]

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

Thank you for your response. Yes, of course

[pitrou]

Thanks for posting this PR and sorry for the delay @andishgar . I added some comments below.

[andishgar]

@pitrou Thanks for the review. I’ll look into it and follow up soon.

[andishgar]

@pitrou
Note: This is unrelated to the PR.
I plan to contribute to Arrow until August 2, and then I will resume contributions from August 30.
If you think there are any issues or pull requests related to me that have higher priority and should be addressed before August 2, please let me know so I can take care of them in time.

Another Note
If you plan to review any of my other PRs, I’d really appreciate it if you could let me know beforehand. I’d like to take a final look and possibly make some improvements first.
(I’ve had a few valuable code review sessions with Kou and learned quite a bit through them. Since most of my code was written before those PRs , I’d prefer to apply those learnings before others take the time to review my work.)

[andishgar]

@pitrou I’ve applied your suggestion. The changes are ready for review.

[andishgar]

Thank you for your feedback. I will look into this.

[andishgar]

@pitrou I applied your suggestions.
Two notes:

1-Regarding this comment, my current algorithm for adding intervals even updates interval.start, so I believe the suggestion to use a std::map doesn't apply in this case. However, if you're still in favor of using std::map, please let me know, and I’ll adjust the logic accordingly.

2- I use a lot of if statements in TryFastInsert for MergeOrInsertInterval, and the tests I wrote cover all the branches. However, if you believe the number of if statements should be reduced, it is possible, since most of them are independent of each other. That said, it's also possible to increase the number of if statements—for example, in every place I use std::max, it could be broken down into several ifs. Another example is the "less than" case in TryFastInsert, which I mentioned in my comments.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

[andishgar]

Rationale for this change

As we discussed here, the following method copies only data that is used in data buffers to a new buffer

What changes are included in this PR?

Add a new method the name of which is CompactArray

Are these changes tested?

I run the relevant unit tests

Are there any user-facing changes?

Yes, I add ComapctArray method to BinaryViewArray class

• GitHub Issue: [C++][Compute] Reducing memory usage when casting from other String types to StringView #46128

[github-actions]

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

[andishgar]

@pitrou When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do? Should I drop this PR?

[andishgar]

Another note:
I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

#include<iostream>
#include<arrow/api.h>
arrow::Status Run() {
int32_t length = std::numeric_limits<int32_t>::max();
std::string input( length,'a');
arrow::StringViewBuilder builder;
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_ASSIGN_OR_RAISE(auto result, builder.Finish());
arrow::ArraySpan span(*result->data());
// The following line ends to Capacity errorARROW_RETURN_NOT_OK(builder.AppendArraySlice(span,0,2));
returnarrow::Status::OK();
}
intmain() {
auto status = Run();
if (!status.ok()) {
std::cerr << status.ToString() << std::endl;
}
return0;
}

Error

Capacity error: BinaryView or StringView elements cannot reference strings larger than 2GB

[pitrou]

When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do?

I still think it's a useful utility method to have. cc @bkietz for additional opinions.

I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

[andishgar]

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

Thank you for your response. Yes, of course

[pitrou]

Thanks for posting this PR and sorry for the delay @andishgar . I added some comments below.

[andishgar]

@pitrou Thanks for the review. I’ll look into it and follow up soon.

[andishgar]

@pitrou
Note: This is unrelated to the PR.
I plan to contribute to Arrow until August 2, and then I will resume contributions from August 30.
If you think there are any issues or pull requests related to me that have higher priority and should be addressed before August 2, please let me know so I can take care of them in time.

Another Note
If you plan to review any of my other PRs, I’d really appreciate it if you could let me know beforehand. I’d like to take a final look and possibly make some improvements first.
(I’ve had a few valuable code review sessions with Kou and learned quite a bit through them. Since most of my code was written before those PRs , I’d prefer to apply those learnings before others take the time to review my work.)

[andishgar]

@pitrou I’ve applied your suggestion. The changes are ready for review.

[andishgar]

Thank you for your feedback. I will look into this.

[andishgar]

@pitrou I applied your suggestions.
Two notes:

1-Regarding this comment, my current algorithm for adding intervals even updates interval.start, so I believe the suggestion to use a std::map doesn't apply in this case. However, if you're still in favor of using std::map, please let me know, and I’ll adjust the logic accordingly.

2- I use a lot of if statements in TryFastInsert for MergeOrInsertInterval, and the tests I wrote cover all the branches. However, if you believe the number of if statements should be reduced, it is possible, since most of them are independent of each other. That said, it's also possible to increase the number of if statements—for example, in every place I use std::max, it could be broken down into several ifs. Another example is the "less than" case in TryFastInsert, which I mentioned in my comments.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

[andishgar]

Rationale for this change

As we discussed here, the following method copies only data that is used in data buffers to a new buffer

What changes are included in this PR?

Add a new method the name of which is CompactArray

Are these changes tested?

I run the relevant unit tests

Are there any user-facing changes?

Yes, I add ComapctArray method to BinaryViewArray class

• GitHub Issue: [C++][Compute] Reducing memory usage when casting from other String types to StringView #46128

[github-actions]

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

[andishgar]

@pitrou When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do? Should I drop this PR?

[andishgar]

Another note:
I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

#include<iostream>
#include<arrow/api.h>
arrow::Status Run() {
int32_t length = std::numeric_limits<int32_t>::max();
std::string input( length,'a');
arrow::StringViewBuilder builder;
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_ASSIGN_OR_RAISE(auto result, builder.Finish());
arrow::ArraySpan span(*result->data());
// The following line ends to Capacity errorARROW_RETURN_NOT_OK(builder.AppendArraySlice(span,0,2));
returnarrow::Status::OK();
}
intmain() {
auto status = Run();
if (!status.ok()) {
std::cerr << status.ToString() << std::endl;
}
return0;
}

Error

Capacity error: BinaryView or StringView elements cannot reference strings larger than 2GB

[pitrou]

When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do?

I still think it's a useful utility method to have. cc @bkietz for additional opinions.

I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

[andishgar]

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

Thank you for your response. Yes, of course

[pitrou]

Thanks for posting this PR and sorry for the delay @andishgar . I added some comments below.

[andishgar]

@pitrou Thanks for the review. I’ll look into it and follow up soon.

[andishgar]

@pitrou
Note: This is unrelated to the PR.
I plan to contribute to Arrow until August 2, and then I will resume contributions from August 30.
If you think there are any issues or pull requests related to me that have higher priority and should be addressed before August 2, please let me know so I can take care of them in time.

Another Note
If you plan to review any of my other PRs, I’d really appreciate it if you could let me know beforehand. I’d like to take a final look and possibly make some improvements first.
(I’ve had a few valuable code review sessions with Kou and learned quite a bit through them. Since most of my code was written before those PRs , I’d prefer to apply those learnings before others take the time to review my work.)

[andishgar]

@pitrou I’ve applied your suggestion. The changes are ready for review.

[andishgar]

Thank you for your feedback. I will look into this.

[andishgar]

@pitrou I applied your suggestions.
Two notes:

1-Regarding this comment, my current algorithm for adding intervals even updates interval.start, so I believe the suggestion to use a std::map doesn't apply in this case. However, if you're still in favor of using std::map, please let me know, and I’ll adjust the logic accordingly.

2- I use a lot of if statements in TryFastInsert for MergeOrInsertInterval, and the tests I wrote cover all the branches. However, if you believe the number of if statements should be reduced, it is possible, since most of them are independent of each other. That said, it's also possible to increase the number of if statements—for example, in every place I use std::max, it could be broken down into several ifs. Another example is the "less than" case in TryFastInsert, which I mentioned in my comments.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

[andishgar]

Rationale for this change

As we discussed here, the following method copies only data that is used in data buffers to a new buffer

What changes are included in this PR?

Add a new method the name of which is CompactArray

Are these changes tested?

I run the relevant unit tests

Are there any user-facing changes?

Yes, I add ComapctArray method to BinaryViewArray class

• GitHub Issue: [C++][Compute] Reducing memory usage when casting from other String types to StringView #46128

[github-actions]

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

[andishgar]

@pitrou When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do? Should I drop this PR?

[andishgar]

Another note:
I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

#include<iostream>
#include<arrow/api.h>
arrow::Status Run() {
int32_t length = std::numeric_limits<int32_t>::max();
std::string input( length,'a');
arrow::StringViewBuilder builder;
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_ASSIGN_OR_RAISE(auto result, builder.Finish());
arrow::ArraySpan span(*result->data());
// The following line ends to Capacity errorARROW_RETURN_NOT_OK(builder.AppendArraySlice(span,0,2));
returnarrow::Status::OK();
}
intmain() {
auto status = Run();
if (!status.ok()) {
std::cerr << status.ToString() << std::endl;
}
return0;
}

Error

Capacity error: BinaryView or StringView elements cannot reference strings larger than 2GB

[pitrou]

When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do?

I still think it's a useful utility method to have. cc @bkietz for additional opinions.

I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

[andishgar]

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

Thank you for your response. Yes, of course

[pitrou]

Thanks for posting this PR and sorry for the delay @andishgar . I added some comments below.

[andishgar]

@pitrou Thanks for the review. I’ll look into it and follow up soon.

[andishgar]

@pitrou
Note: This is unrelated to the PR.
I plan to contribute to Arrow until August 2, and then I will resume contributions from August 30.
If you think there are any issues or pull requests related to me that have higher priority and should be addressed before August 2, please let me know so I can take care of them in time.

Another Note
If you plan to review any of my other PRs, I’d really appreciate it if you could let me know beforehand. I’d like to take a final look and possibly make some improvements first.
(I’ve had a few valuable code review sessions with Kou and learned quite a bit through them. Since most of my code was written before those PRs , I’d prefer to apply those learnings before others take the time to review my work.)

[andishgar]

@pitrou I’ve applied your suggestion. The changes are ready for review.

[andishgar]

Thank you for your feedback. I will look into this.

[andishgar]

@pitrou I applied your suggestions.
Two notes:

1-Regarding this comment, my current algorithm for adding intervals even updates interval.start, so I believe the suggestion to use a std::map doesn't apply in this case. However, if you're still in favor of using std::map, please let me know, and I’ll adjust the logic accordingly.

2- I use a lot of if statements in TryFastInsert for MergeOrInsertInterval, and the tests I wrote cover all the branches. However, if you believe the number of if statements should be reduced, it is possible, since most of them are independent of each other. That said, it's also possible to increase the number of if statements—for example, in every place I use std::max, it could be broken down into several ifs. Another example is the "less than" case in TryFastInsert, which I mentioned in my comments.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

[andishgar]

Rationale for this change

As we discussed here, the following method copies only data that is used in data buffers to a new buffer

What changes are included in this PR?

Add a new method the name of which is CompactArray

Are these changes tested?

I run the relevant unit tests

Are there any user-facing changes?

Yes, I add ComapctArray method to BinaryViewArray class

• GitHub Issue: [C++][Compute] Reducing memory usage when casting from other String types to StringView #46128

[github-actions]

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

[andishgar]

@pitrou When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do? Should I drop this PR?

[andishgar]

Another note:
I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

#include<iostream>
#include<arrow/api.h>
arrow::Status Run() {
int32_t length = std::numeric_limits<int32_t>::max();
std::string input( length,'a');
arrow::StringViewBuilder builder;
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_ASSIGN_OR_RAISE(auto result, builder.Finish());
arrow::ArraySpan span(*result->data());
// The following line ends to Capacity errorARROW_RETURN_NOT_OK(builder.AppendArraySlice(span,0,2));
returnarrow::Status::OK();
}
intmain() {
auto status = Run();
if (!status.ok()) {
std::cerr << status.ToString() << std::endl;
}
return0;
}

Error

Capacity error: BinaryView or StringView elements cannot reference strings larger than 2GB

[pitrou]

When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do?

I still think it's a useful utility method to have. cc @bkietz for additional opinions.

I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

[andishgar]

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

Thank you for your response. Yes, of course

[pitrou]

Thanks for posting this PR and sorry for the delay @andishgar . I added some comments below.

[andishgar]

@pitrou Thanks for the review. I’ll look into it and follow up soon.

[andishgar]

@pitrou
Note: This is unrelated to the PR.
I plan to contribute to Arrow until August 2, and then I will resume contributions from August 30.
If you think there are any issues or pull requests related to me that have higher priority and should be addressed before August 2, please let me know so I can take care of them in time.

Another Note
If you plan to review any of my other PRs, I’d really appreciate it if you could let me know beforehand. I’d like to take a final look and possibly make some improvements first.
(I’ve had a few valuable code review sessions with Kou and learned quite a bit through them. Since most of my code was written before those PRs , I’d prefer to apply those learnings before others take the time to review my work.)

[andishgar]

@pitrou I’ve applied your suggestion. The changes are ready for review.

[andishgar]

Thank you for your feedback. I will look into this.

[andishgar]

@pitrou I applied your suggestions.
Two notes:

1-Regarding this comment, my current algorithm for adding intervals even updates interval.start, so I believe the suggestion to use a std::map doesn't apply in this case. However, if you're still in favor of using std::map, please let me know, and I’ll adjust the logic accordingly.

2- I use a lot of if statements in TryFastInsert for MergeOrInsertInterval, and the tests I wrote cover all the branches. However, if you believe the number of if statements should be reduced, it is possible, since most of them are independent of each other. That said, it's also possible to increase the number of if statements—for example, in every place I use std::max, it could be broken down into several ifs. Another example is the "less than" case in TryFastInsert, which I mentioned in my comments.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

[andishgar]

Rationale for this change

As we discussed here, the following method copies only data that is used in data buffers to a new buffer

What changes are included in this PR?

Add a new method the name of which is CompactArray

Are these changes tested?

I run the relevant unit tests

Are there any user-facing changes?

Yes, I add ComapctArray method to BinaryViewArray class

• GitHub Issue: [C++][Compute] Reducing memory usage when casting from other String types to StringView #46128

[github-actions]

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

[andishgar]

@pitrou When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do? Should I drop this PR?

[andishgar]

Another note:
I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

#include<iostream>
#include<arrow/api.h>
arrow::Status Run() {
int32_t length = std::numeric_limits<int32_t>::max();
std::string input( length,'a');
arrow::StringViewBuilder builder;
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_ASSIGN_OR_RAISE(auto result, builder.Finish());
arrow::ArraySpan span(*result->data());
// The following line ends to Capacity errorARROW_RETURN_NOT_OK(builder.AppendArraySlice(span,0,2));
returnarrow::Status::OK();
}
intmain() {
auto status = Run();
if (!status.ok()) {
std::cerr << status.ToString() << std::endl;
}
return0;
}

Error

Capacity error: BinaryView or StringView elements cannot reference strings larger than 2GB

[pitrou]

When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do?

I still think it's a useful utility method to have. cc @bkietz for additional opinions.

I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

[andishgar]

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

Thank you for your response. Yes, of course

[pitrou]

Thanks for posting this PR and sorry for the delay @andishgar . I added some comments below.

[andishgar]

@pitrou Thanks for the review. I’ll look into it and follow up soon.

[andishgar]

@pitrou
Note: This is unrelated to the PR.
I plan to contribute to Arrow until August 2, and then I will resume contributions from August 30.
If you think there are any issues or pull requests related to me that have higher priority and should be addressed before August 2, please let me know so I can take care of them in time.

Another Note
If you plan to review any of my other PRs, I’d really appreciate it if you could let me know beforehand. I’d like to take a final look and possibly make some improvements first.
(I’ve had a few valuable code review sessions with Kou and learned quite a bit through them. Since most of my code was written before those PRs , I’d prefer to apply those learnings before others take the time to review my work.)

[andishgar]

@pitrou I’ve applied your suggestion. The changes are ready for review.

[andishgar]

Thank you for your feedback. I will look into this.

[andishgar]

@pitrou I applied your suggestions.
Two notes:

1-Regarding this comment, my current algorithm for adding intervals even updates interval.start, so I believe the suggestion to use a std::map doesn't apply in this case. However, if you're still in favor of using std::map, please let me know, and I’ll adjust the logic accordingly.

2- I use a lot of if statements in TryFastInsert for MergeOrInsertInterval, and the tests I wrote cover all the branches. However, if you believe the number of if statements should be reduced, it is possible, since most of them are independent of each other. That said, it's also possible to increase the number of if statements—for example, in every place I use std::max, it could be broken down into several ifs. Another example is the "less than" case in TryFastInsert, which I mentioned in my comments.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.

[andishgar]

Rationale for this change

As we discussed here, the following method copies only data that is used in data buffers to a new buffer

What changes are included in this PR?

Add a new method the name of which is CompactArray

Are these changes tested?

I run the relevant unit tests

Are there any user-facing changes?

Yes, I add ComapctArray method to BinaryViewArray class

• GitHub Issue: [C++][Compute] Reducing memory usage when casting from other String types to StringView #46128

[github-actions]

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

[andishgar]

@pitrou When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do? Should I drop this PR?

[andishgar]

Another note:
I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

#include<iostream>
#include<arrow/api.h>
arrow::Status Run() {
int32_t length = std::numeric_limits<int32_t>::max();
std::string input( length,'a');
arrow::StringViewBuilder builder;
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_RETURN_NOT_OK(builder.Append(input));
ARROW_ASSIGN_OR_RAISE(auto result, builder.Finish());
arrow::ArraySpan span(*result->data());
// The following line ends to Capacity errorARROW_RETURN_NOT_OK(builder.AppendArraySlice(span,0,2));
returnarrow::Status::OK();
}
intmain() {
auto status = Run();
if (!status.ok()) {
std::cerr << status.ToString() << std::endl;
}
return0;
}

Error

Capacity error: BinaryView or StringView elements cannot reference strings larger than 2GB

[pitrou]

When I suggested adding these methods, I wasn't aware thatarrow::BinaryViewBuilder::AppendArraySlice already serves the same purpose. What do you recommend I do?

I still think it's a useful utility method to have. cc @bkietz for additional opinions.

I found that the following code results in an error when using arrow::BinaryViewBuilder::AppendArraySlice. Is there any priority or plan to address this?

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

[andishgar]

I don't think this bug is already reported, so there is no "priority or plan". Can you open a separate issue for it, and perhaps submit a PR if you're motivated?

Thank you for your response. Yes, of course

[pitrou]

Thanks for posting this PR and sorry for the delay @andishgar . I added some comments below.

[andishgar]

@pitrou Thanks for the review. I’ll look into it and follow up soon.

[andishgar]

@pitrou
Note: This is unrelated to the PR.
I plan to contribute to Arrow until August 2, and then I will resume contributions from August 30.
If you think there are any issues or pull requests related to me that have higher priority and should be addressed before August 2, please let me know so I can take care of them in time.

Another Note
If you plan to review any of my other PRs, I’d really appreciate it if you could let me know beforehand. I’d like to take a final look and possibly make some improvements first.
(I’ve had a few valuable code review sessions with Kou and learned quite a bit through them. Since most of my code was written before those PRs , I’d prefer to apply those learnings before others take the time to review my work.)

[andishgar]

@pitrou I’ve applied your suggestion. The changes are ready for review.

[andishgar]

Thank you for your feedback. I will look into this.

[andishgar]

@pitrou I applied your suggestions.
Two notes:

1-Regarding this comment, my current algorithm for adding intervals even updates interval.start, so I believe the suggestion to use a std::map doesn't apply in this case. However, if you're still in favor of using std::map, please let me know, and I’ll adjust the logic accordingly.

2- I use a lot of if statements in TryFastInsert for MergeOrInsertInterval, and the tests I wrote cover all the branches. However, if you believe the number of if statements should be reduced, it is possible, since most of them are independent of each other. That said, it's also possible to increase the number of if statements—for example, in every place I use std::max, it could be broken down into several ifs. Another example is the "less than" case in TryFastInsert, which I mentioned in my comments.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.