You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2024/02/23 03:14:51 UTC
[PR] Take ccc [arrow]
felipecrv opened a new pull request, #40206:
URL: https://github.com/apache/arrow/pull/40206
### Rationale for this change
`take` concatenates chunks when it's applied to a chunked `values` array, but when the `indices` arrays is also `chunked` it concatenates `values` more than once -- one `Concatenate` call with `values.chunks()` for every chunk in `indices`. This PR doesn't remove the concatenation, but ensures it's done only once instead of `indices.size()` times.
### What changes are included in this PR?
- Adding return type to the `TakeXX` names (-> `TakeXXY`) to makes code easier to understand
- Adding benchmarks to `TakeCCC` — copied from #13857
- Remove the concatenation from the loop body (!)
### Are these changes tested?
By existing tests.
### Are there any user-facing changes?
A faster compute kernel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1966548768
@js8544 thank you for the review. I pushed a revert for the last commit because it isn't necessary for *this* PR and might conflict with a PR someone else is working on.
If not one objects, I will probably merge this right before I'm ready to send more Take-related PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1504236409
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -158,6 +170,39 @@ struct TakeBenchmark {
}
state.SetItemsProcessed(state.iterations() * values->length());
}
+
+ void BenchChunked(const std::shared_ptr<ChunkedArray>& values, bool chunk_indices_too) {
+ double indices_null_proportion = indices_have_nulls ? args.null_proportion : 0;
+ auto indices =
+ rand.Int32(values->length(), 0, static_cast<int32_t>(values->length() - 1),
+ indices_null_proportion);
+
+ if (monotonic_indices) {
+ auto arg_sorter = *SortIndices(*indices);
+ indices = *Take(*indices, *arg_sorter);
+ }
+ std::shared_ptr<ChunkedArray> chunked_indices;
+ if (chunk_indices_too) {
+ std::vector<std::shared_ptr<Array>> indices_chunks;
+ int64_t offset = 0;
+ for (int i = 0; i < values->num_chunks(); ++i) {
+ auto chunk = indices->Slice(offset, values->chunk(i)->length());
+ indices_chunks.push_back(std::move(chunk));
+ offset += values->chunk(i)->length();
+ }
+ chunked_indices = std::make_shared<ChunkedArray>(std::move(indices_chunks));
+ }
+
+ if (chunk_indices_too) {
+ for (auto _ : state) {
+ ABORT_NOT_OK(Take(values, chunked_indices).status());
+ }
+ } else {
+ for (auto _ : state) {
+ ABORT_NOT_OK(Take(values, indices).status());
+ }
+ }
Review Comment:
Can we also call `SetItemsProcessed` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #40206:
URL: https://github.com/apache/arrow/pull/40206
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1504254087
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -158,6 +170,39 @@ struct TakeBenchmark {
}
state.SetItemsProcessed(state.iterations() * values->length());
}
+
+ void BenchChunked(const std::shared_ptr<ChunkedArray>& values, bool chunk_indices_too) {
+ double indices_null_proportion = indices_have_nulls ? args.null_proportion : 0;
+ auto indices =
+ rand.Int32(values->length(), 0, static_cast<int32_t>(values->length() - 1),
+ indices_null_proportion);
+
+ if (monotonic_indices) {
+ auto arg_sorter = *SortIndices(*indices);
+ indices = *Take(*indices, *arg_sorter);
+ }
+ std::shared_ptr<ChunkedArray> chunked_indices;
+ if (chunk_indices_too) {
+ std::vector<std::shared_ptr<Array>> indices_chunks;
+ int64_t offset = 0;
+ for (int i = 0; i < values->num_chunks(); ++i) {
+ auto chunk = indices->Slice(offset, values->chunk(i)->length());
+ indices_chunks.push_back(std::move(chunk));
+ offset += values->chunk(i)->length();
+ }
+ chunked_indices = std::make_shared<ChunkedArray>(std::move(indices_chunks));
+ }
+
+ if (chunk_indices_too) {
+ for (auto _ : state) {
+ ABORT_NOT_OK(Take(values, chunked_indices).status());
+ }
+ } else {
+ for (auto _ : state) {
+ ABORT_NOT_OK(Take(values, indices).status());
+ }
+ }
Review Comment:
For some reason, `items_per_second` appears in the benchmark output even though `SetItemsProcessed` does not seem called explicitly. Am I missing something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1966570687
The added benchmarks are improved a lot by the algorithmic changes. Neat!
```
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Non-regressions: (30)
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
benchmark baseline contender change % counters
TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/1 42.356M items/sec 990.369M items/sec 2238.183 {'family_index': 1, 'per_family_instance_index': 3, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/1', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 57, 'null_percent': 100.0}
TakeChunkedFlatInt64RandomIndicesWithNulls/524288/1 44.822M items/sec 968.170M items/sec 2060.035 {'family_index': 4, 'per_family_instance_index': 3, 'run_name': 'TakeChunkedFlatInt64RandomIndicesWithNulls/524288/1', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 59, 'null_percent': 100.0}
TakeChunkedChunkedInt64MonotonicIndices/524288/1 41.670M items/sec 427.030M items/sec 924.791 {'family_index': 2, 'per_family_instance_index': 3, 'run_name': 'TakeChunkedChunkedInt64MonotonicIndices/524288/1', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 56, 'null_percent': 100.0}
TakeChunkedFlatInt64MonotonicIndices/524288/1 43.028M items/sec 434.987M items/sec 910.949 {'family_index': 5, 'per_family_instance_index': 3, 'run_name': 'TakeChunkedFlatInt64MonotonicIndices/524288/1', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 57, 'null_percent': 100.0}
TakeChunkedFlatInt64RandomIndicesNoNulls/524288/1 41.435M items/sec 416.685M items/sec 905.646 {'family_index': 3, 'per_family_instance_index': 3, 'run_name': 'TakeChunkedFlatInt64RandomIndicesNoNulls/524288/1', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 55, 'null_percent': 100.0}
TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/1 41.287M items/sec 413.244M items/sec 900.913 {'family_index': 0, 'per_family_instance_index': 3, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/1', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 55, 'null_percent': 100.0}
TakeChunkedChunkedInt64MonotonicIndices/524288/0 47.665M items/sec 470.231M items/sec 886.531 {'family_index': 2, 'per_family_instance_index': 4, 'run_name': 'TakeChunkedChunkedInt64MonotonicIndices/524288/0', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 64, 'null_percent': 0.0}
TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/0 44.595M items/sec 424.955M items/sec 852.910 {'family_index': 1, 'per_family_instance_index': 4, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/0', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 61, 'null_percent': 0.0}
TakeChunkedFlatInt64MonotonicIndices/524288/0 48.917M items/sec 465.193M items/sec 850.976 {'family_index': 5, 'per_family_instance_index': 4, 'run_name': 'TakeChunkedFlatInt64MonotonicIndices/524288/0', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 65, 'null_percent': 0.0}
TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/0 47.166M items/sec 414.813M items/sec 779.479 {'family_index': 0, 'per_family_instance_index': 4, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/0', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 63, 'null_percent': 0.0}
TakeChunkedFlatInt64RandomIndicesNoNulls/524288/0 48.511M items/sec 422.180M items/sec 770.278 {'family_index': 3, 'per_family_instance_index': 4, 'run_name': 'TakeChunkedFlatInt64RandomIndicesNoNulls/524288/0', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 63, 'null_percent': 0.0}
TakeChunkedFlatInt64RandomIndicesWithNulls/524288/0 48.690M items/sec 416.701M items/sec 755.819 {'family_index': 4, 'per_family_instance_index': 4, 'run_name': 'TakeChunkedFlatInt64RandomIndicesWithNulls/524288/0', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 64, 'null_percent': 0.0}
TakeChunkedChunkedInt64MonotonicIndices/524288/1000 40.203M items/sec 306.199M items/sec 661.631 {'family_index': 2, 'per_family_instance_index': 0, 'run_name': 'TakeChunkedChunkedInt64MonotonicIndices/524288/1000', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 53, 'null_percent': 0.1}
TakeChunkedFlatInt64MonotonicIndices/524288/1000 41.141M items/sec 303.560M items/sec 637.846 {'family_index': 5, 'per_family_instance_index': 0, 'run_name': 'TakeChunkedFlatInt64MonotonicIndices/524288/1000', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 55, 'null_percent': 0.1}
TakeChunkedChunkedInt64MonotonicIndices/524288/10 38.784M items/sec 254.283M items/sec 555.638 {'family_index': 2, 'per_family_instance_index': 1, 'run_name': 'TakeChunkedChunkedInt64MonotonicIndices/524288/10', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 52, 'null_percent': 10.0}
TakeChunkedFlatInt64MonotonicIndices/524288/10 40.112M items/sec 254.035M items/sec 533.320 {'family_index': 5, 'per_family_instance_index': 1, 'run_name': 'TakeChunkedFlatInt64MonotonicIndices/524288/10', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 53, 'null_percent': 10.0}
TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/1000 37.405M items/sec 228.050M items/sec 509.682 {'family_index': 0, 'per_family_instance_index': 0, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/1000', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 46, 'null_percent': 0.1}
TakeChunkedFlatInt64RandomIndicesNoNulls/524288/1000 37.929M items/sec 230.621M items/sec 508.029 {'family_index': 3, 'per_family_instance_index': 0, 'run_name': 'TakeChunkedFlatInt64RandomIndicesNoNulls/524288/1000', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 51, 'null_percent': 0.1}
TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/1000 37.423M items/sec 217.546M items/sec 481.314 {'family_index': 1, 'per_family_instance_index': 0, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/1000', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 50, 'null_percent': 0.1}
TakeChunkedFlatInt64RandomIndicesWithNulls/524288/1000 37.925M items/sec 218.967M items/sec 477.367 {'family_index': 4, 'per_family_instance_index': 0, 'run_name': 'TakeChunkedFlatInt64RandomIndicesWithNulls/524288/1000', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 51, 'null_percent': 0.1}
TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/10 37.092M items/sec 209.418M items/sec 464.590 {'family_index': 0, 'per_family_instance_index': 1, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/10', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 49, 'null_percent': 10.0}
TakeChunkedFlatInt64RandomIndicesNoNulls/524288/10 38.211M items/sec 210.413M items/sec 450.660 {'family_index': 3, 'per_family_instance_index': 1, 'run_name': 'TakeChunkedFlatInt64RandomIndicesNoNulls/524288/10', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 51, 'null_percent': 10.0}
TakeChunkedChunkedInt64MonotonicIndices/524288/2 36.703M items/sec 189.921M items/sec 417.456 {'family_index': 2, 'per_family_instance_index': 2, 'run_name': 'TakeChunkedChunkedInt64MonotonicIndices/524288/2', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 50, 'null_percent': 50.0}
TakeChunkedFlatInt64MonotonicIndices/524288/2 38.049M items/sec 188.632M items/sec 395.761 {'family_index': 5, 'per_family_instance_index': 2, 'run_name': 'TakeChunkedFlatInt64MonotonicIndices/524288/2', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 51, 'null_percent': 50.0}
TakeChunkedFlatInt64RandomIndicesNoNulls/524288/2 35.089M items/sec 152.790M items/sec 335.432 {'family_index': 3, 'per_family_instance_index': 2, 'run_name': 'TakeChunkedFlatInt64RandomIndicesNoNulls/524288/2', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 46, 'null_percent': 50.0}
TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/2 34.968M items/sec 152.119M items/sec 335.027 {'family_index': 0, 'per_family_instance_index': 2, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesNoNulls/524288/2', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 47, 'null_percent': 50.0}
TakeChunkedFlatInt64RandomIndicesWithNulls/524288/10 35.519M items/sec 152.181M items/sec 328.450 {'family_index': 4, 'per_family_instance_index': 1, 'run_name': 'TakeChunkedFlatInt64RandomIndicesWithNulls/524288/10', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 47, 'null_percent': 10.0}
TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/10 34.179M items/sec 134.705M items/sec 294.115 {'family_index': 1, 'per_family_instance_index': 1, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/10', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 46, 'null_percent': 10.0}
TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/2 30.641M items/sec 100.546M items/sec 228.138 {'family_index': 1, 'per_family_instance_index': 2, 'run_name': 'TakeChunkedChunkedInt64RandomIndicesWithNulls/524288/2', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 41, 'null_percent': 50.0}
TakeChunkedFlatInt64RandomIndicesWithNulls/524288/2 31.772M items/sec 100.508M items/sec 216.344 {'family_index': 4, 'per_family_instance_index': 2, 'run_name': 'TakeChunkedFlatInt64RandomIndicesWithNulls/524288/2', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 42, 'null_percent': 50.0}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1969664783
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 2c13a19fd31063a0426ae85b923f9cb0f523acf4.
There were 2 benchmark results with an error:
- Commit Run on `ursa-i9-9960x` at [2024-02-28 12:08:00Z](https://conbench.ursa.dev/compare/runs/26ee9e776e8e409fa9e63e059607fe59...1a02470c7cf44a6389ff827454bbdbc6/)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-12, scale_factor=10](https://conbench.ursa.dev/benchmark-results/065df3a0df5a7eab8000d81130cb023d)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-05, scale_factor=10](https://conbench.ursa.dev/benchmark-results/065df373450c73e38000d474bd848781)
There were 13 benchmark results indicating a performance regression:
- Commit Run on `ursa-i9-9960x` at [2024-02-28 12:08:00Z](https://conbench.ursa.dev/compare/runs/26ee9e776e8e409fa9e63e059607fe59...1a02470c7cf44a6389ff827454bbdbc6/)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-20, scale_factor=1](https://conbench.ursa.dev/compare/benchmarks/065df1ab987a71c28000f07e34ccb9b8...065df3d88fe876218000b1c6808809ba)
- [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-01, scale_factor=1](https://conbench.ursa.dev/compare/benchmarks/065df13ac15e7d1b8000708ae5209229...065df35a2ffb78b28000ff49cab8da51)
- and 11 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/22094018786) has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1505070414
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -322,6 +367,36 @@ static void TakeFixedSizeBinaryMonotonicIndices(benchmark::State& state) {
.FixedSizeBinary();
}
+static void TakeChunkedChunkedInt64RandomIndicesNoNulls(benchmark::State& state) {
+ TakeBenchmark(state, false)
Review Comment:
I will add to this and all the other cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1960678117
Items per second improved a lot. Higher is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1505068427
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -158,6 +170,39 @@ struct TakeBenchmark {
}
state.SetItemsProcessed(state.iterations() * values->length());
}
+
+ void BenchChunked(const std::shared_ptr<ChunkedArray>& values, bool chunk_indices_too) {
+ double indices_null_proportion = indices_have_nulls ? args.null_proportion : 0;
+ auto indices =
+ rand.Int32(values->length(), 0, static_cast<int32_t>(values->length() - 1),
+ indices_null_proportion);
+
+ if (monotonic_indices) {
+ auto arg_sorter = *SortIndices(*indices);
+ indices = *Take(*indices, *arg_sorter);
+ }
+ std::shared_ptr<ChunkedArray> chunked_indices;
+ if (chunk_indices_too) {
+ std::vector<std::shared_ptr<Array>> indices_chunks;
+ int64_t offset = 0;
+ for (int i = 0; i < values->num_chunks(); ++i) {
+ auto chunk = indices->Slice(offset, values->chunk(i)->length());
+ indices_chunks.push_back(std::move(chunk));
+ offset += values->chunk(i)->length();
+ }
+ chunked_indices = std::make_shared<ChunkedArray>(std::move(indices_chunks));
+ }
+
+ if (chunk_indices_too) {
+ for (auto _ : state) {
+ ABORT_NOT_OK(Take(values, chunked_indices).status());
+ }
+ } else {
+ for (auto _ : state) {
+ ABORT_NOT_OK(Take(values, indices).status());
+ }
+ }
Review Comment:
Well... I'm going to add the explicit call. I copied these benchmarks from another PR and modified them, but missed this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1505071431
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -408,6 +483,12 @@ void TakeFSBSetArgs(benchmark::internal::Benchmark* bench) {
BENCHMARK(TakeInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
BENCHMARK(TakeInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
BENCHMARK(TakeInt64MonotonicIndices)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64MonotonicIndices)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64MonotonicIndices)->Apply(TakeSetArgs);
BENCHMARK(TakeFixedSizeBinaryRandomIndicesNoNulls)->Apply(TakeFSBSetArgs);
BENCHMARK(TakeFixedSizeBinaryRandomIndicesWithNulls)->Apply(TakeFSBSetArgs);
BENCHMARK(TakeFixedSizeBinaryMonotonicIndices)->Apply(TakeFSBSetArgs);
Review Comment:
Oops. This is why code review is good :D Fixing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1965725287
+1. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1505073220
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -322,6 +367,36 @@ static void TakeFixedSizeBinaryMonotonicIndices(benchmark::State& state) {
.FixedSizeBinary();
}
+static void TakeChunkedChunkedInt64RandomIndicesNoNulls(benchmark::State& state) {
+ TakeBenchmark(state, false)
+ .ChunkedInt64(/*num_chunks=*/100, /*chunk_indices_too=*/true);
+}
+
+static void TakeChunkedChunkedInt64RandomIndicesWithNulls(benchmark::State& state) {
+ TakeBenchmark(state, true).ChunkedInt64(/*num_chunks=*/100, /*chunk_indices_too=*/true);
+}
+
+static void TakeChunkedChunkedInt64MonotonicIndices(benchmark::State& state) {
+ TakeBenchmark(state, /*indices_with_nulls=*/false, /*monotonic=*/true)
+ .ChunkedInt64(
+ /*num_chunks=*/100, /*chunk_indices_too=*/true);
+}
+
+static void TakeChunkedFlatInt64RandomIndicesNoNulls(benchmark::State& state) {
+ TakeBenchmark(state, false)
+ .ChunkedInt64(/*num_chunks=*/100, /*chunk_indices_too=*/true);
Review Comment:
Yep. It's wrong. Fixing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1960677659
:warning: GitHub issue #40207 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1967387584
> @felipecrv Would you like to rebase if not already done?
Soon. I'm currently working on top of this branch locally 😅
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1504254640
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -408,6 +483,12 @@ void TakeFSBSetArgs(benchmark::internal::Benchmark* bench) {
BENCHMARK(TakeInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
BENCHMARK(TakeInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
BENCHMARK(TakeInt64MonotonicIndices)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64MonotonicIndices)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64MonotonicIndices)->Apply(TakeSetArgs);
BENCHMARK(TakeFixedSizeBinaryRandomIndicesNoNulls)->Apply(TakeFSBSetArgs);
BENCHMARK(TakeFixedSizeBinaryRandomIndicesWithNulls)->Apply(TakeFSBSetArgs);
BENCHMARK(TakeFixedSizeBinaryMonotonicIndices)->Apply(TakeFSBSetArgs);
Review Comment:
("ChunkedFlat" is less interesting arguably)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1504240362
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -408,6 +483,12 @@ void TakeFSBSetArgs(benchmark::internal::Benchmark* bench) {
BENCHMARK(TakeInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
BENCHMARK(TakeInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
BENCHMARK(TakeInt64MonotonicIndices)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64MonotonicIndices)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64MonotonicIndices)->Apply(TakeSetArgs);
BENCHMARK(TakeFixedSizeBinaryRandomIndicesNoNulls)->Apply(TakeFSBSetArgs);
BENCHMARK(TakeFixedSizeBinaryRandomIndicesWithNulls)->Apply(TakeFSBSetArgs);
BENCHMARK(TakeFixedSizeBinaryMonotonicIndices)->Apply(TakeFSBSetArgs);
Review Comment:
Why not add at least "ChunkedChunked" variations for each of these types as well? (FSB, FSL, String). I'm assuming the performance characteristics might be different?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Take ccc [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1960676453
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1962415750
Making the only difference between the benchmarks, the fact that the array is chunked (in a 100 chunks) makes the differences more evident.
<img width="1294" alt="Screenshot 2024-02-23 at 12 57 16" src="https://github.com/apache/arrow/assets/207795/1c2b09db-dfc3-4a66-b5b8-ee8235d91e32">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40206:
URL: https://github.com/apache/arrow/pull/40206#issuecomment-1966586231
@felipecrv Would you like to rebase if not already done?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1504238517
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -322,6 +367,36 @@ static void TakeFixedSizeBinaryMonotonicIndices(benchmark::State& state) {
.FixedSizeBinary();
}
+static void TakeChunkedChunkedInt64RandomIndicesNoNulls(benchmark::State& state) {
+ TakeBenchmark(state, false)
Review Comment:
Can we state bool arg names consistently? I mean `/*indices_with_nulls=*/`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1505071431
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -408,6 +483,12 @@ void TakeFSBSetArgs(benchmark::internal::Benchmark* bench) {
BENCHMARK(TakeInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
BENCHMARK(TakeInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
BENCHMARK(TakeInt64MonotonicIndices)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedChunkedInt64MonotonicIndices)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64RandomIndicesNoNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64RandomIndicesWithNulls)->Apply(TakeSetArgs);
+BENCHMARK(TakeChunkedFlatInt64MonotonicIndices)->Apply(TakeSetArgs);
BENCHMARK(TakeFixedSizeBinaryRandomIndicesNoNulls)->Apply(TakeFSBSetArgs);
BENCHMARK(TakeFixedSizeBinaryRandomIndicesWithNulls)->Apply(TakeFSBSetArgs);
BENCHMARK(TakeFixedSizeBinaryMonotonicIndices)->Apply(TakeFSBSetArgs);
Review Comment:
Oops. This is why code review is good :D Fixing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-40207: [C++] TakeCC: Concatenate only once and delegate to TakeAA instead of TakeCA [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #40206:
URL: https://github.com/apache/arrow/pull/40206#discussion_r1504239196
##########
cpp/src/arrow/compute/kernels/vector_selection_benchmark.cc:
##########
@@ -322,6 +367,36 @@ static void TakeFixedSizeBinaryMonotonicIndices(benchmark::State& state) {
.FixedSizeBinary();
}
+static void TakeChunkedChunkedInt64RandomIndicesNoNulls(benchmark::State& state) {
+ TakeBenchmark(state, false)
+ .ChunkedInt64(/*num_chunks=*/100, /*chunk_indices_too=*/true);
+}
+
+static void TakeChunkedChunkedInt64RandomIndicesWithNulls(benchmark::State& state) {
+ TakeBenchmark(state, true).ChunkedInt64(/*num_chunks=*/100, /*chunk_indices_too=*/true);
+}
+
+static void TakeChunkedChunkedInt64MonotonicIndices(benchmark::State& state) {
+ TakeBenchmark(state, /*indices_with_nulls=*/false, /*monotonic=*/true)
+ .ChunkedInt64(
+ /*num_chunks=*/100, /*chunk_indices_too=*/true);
+}
+
+static void TakeChunkedFlatInt64RandomIndicesNoNulls(benchmark::State& state) {
+ TakeBenchmark(state, false)
+ .ChunkedInt64(/*num_chunks=*/100, /*chunk_indices_too=*/true);
Review Comment:
If this is meant to be "flat", do you mean `/*chunk_indices_too=*/false`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org