MINOR: [C++] Use [] instead of exception-throwing at(i) in concatenate.cc

[felipecrv]

Rationale for this change

vector::at performs bounds checking and can throw an exception [1]. Its use is discouraged and in this specific case, the access is provably safe because array is previously resized to buffers.size().

[1] https://en.cppreference.com/w/cpp/container/vector/at

What changes are included in this PR?

Use of operator[] instead of at().

Are these changes tested?

By the existing concatenation tests.

[felipecrv]

@bkietz

[bkietz]

LGTM

[felipecrv]

Rebased and pushed again to see if all CI can pass now.

[pitrou]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

[mapleFU]

staticvoidVectorIndex(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec[i];
benchmark::DoNotOptimize(v);
}
}
}
// Register the function as a benchmarkBENCHMARK(VectorIndex);
staticvoidVectorAt(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec.at(i);
benchmark::DoNotOptimize(v);
}
}
}
BENCHMARK(VectorAt);

With release(-O2) on my MacOS:

------------------------------------------------------
Benchmark Time CPU Iterations
------------------------------------------------------
VectorIndex 24632 ns 17785 ns 38620
VectorAt 37438 ns 31141 ns 21829

[felipecrv]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

@pitrou C++ lore.

at() is forbidden in exception-free codebases which Arrow mostly is: Status and Result instead of throw everywhere.

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

[pitrou]

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

Ah, that's a good point.

[pitrou]

The "AMD64 Conda C++" CI failure seems unexpected, can you take a look?

[felipecrv]

@pitrou I only found unreleated issues, but one of them is being fixed in mapleFU's PR.

When that is merged I will rebase and force-push here.

[felipecrv]

I pushed a commit here to test a fix for parquet CI tests. If it passes, I will create a separate issue/PR for it.

[pitrou]

Can you rebase on git main so as to get a slightly less failing CI? :-)

[felipecrv]

Can you rebase on git main so as to get a slightly less failing CI? :-)

I'm waiting for @mapleFU's PR to be merged.

[pitrou]

CI failures are unrelated, will merge.

[ursabot]

Benchmark runs are scheduled for baseline = 584dc7b and contender = 2216a0a. 2216a0a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.86% ⬆️0.09%] test-mac-arm
[Finished ⬇️1.63% ⬆️4.58%] ursa-i9-9960x
[Finished ⬇️0.52% ⬆️1.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2216a0a4 ec2-t3-xlarge-us-east-2
[Finished] 2216a0a4 test-mac-arm
[Finished] 2216a0a4 ursa-i9-9960x
[Finished] 2216a0a4 ursa-thinkcentre-m75q
[Finished] 584dc7bc ec2-t3-xlarge-us-east-2
[Finished] 584dc7bc test-mac-arm
[Finished] 584dc7bc ursa-i9-9960x
[Finished] 584dc7bc ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[felipecrv]

Rationale for this change

vector::at performs bounds checking and can throw an exception [1]. Its use is discouraged and in this specific case, the access is provably safe because array is previously resized to buffers.size().

[1] https://en.cppreference.com/w/cpp/container/vector/at

What changes are included in this PR?

Use of operator[] instead of at().

Are these changes tested?

By the existing concatenation tests.

[felipecrv]

@bkietz

[bkietz]

LGTM

[felipecrv]

Rebased and pushed again to see if all CI can pass now.

[pitrou]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

[mapleFU]

staticvoidVectorIndex(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec[i];
benchmark::DoNotOptimize(v);
}
}
}
// Register the function as a benchmarkBENCHMARK(VectorIndex);
staticvoidVectorAt(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec.at(i);
benchmark::DoNotOptimize(v);
}
}
}
BENCHMARK(VectorAt);

With release(-O2) on my MacOS:

------------------------------------------------------
Benchmark Time CPU Iterations
------------------------------------------------------
VectorIndex 24632 ns 17785 ns 38620
VectorAt 37438 ns 31141 ns 21829

[felipecrv]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

@pitrou C++ lore.

at() is forbidden in exception-free codebases which Arrow mostly is: Status and Result instead of throw everywhere.

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

[pitrou]

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

Ah, that's a good point.

[pitrou]

The "AMD64 Conda C++" CI failure seems unexpected, can you take a look?

[felipecrv]

@pitrou I only found unreleated issues, but one of them is being fixed in mapleFU's PR.

When that is merged I will rebase and force-push here.

[felipecrv]

I pushed a commit here to test a fix for parquet CI tests. If it passes, I will create a separate issue/PR for it.

[pitrou]

Can you rebase on git main so as to get a slightly less failing CI? :-)

[felipecrv]

Can you rebase on git main so as to get a slightly less failing CI? :-)

I'm waiting for @mapleFU's PR to be merged.

[pitrou]

CI failures are unrelated, will merge.

[ursabot]

Benchmark runs are scheduled for baseline = 584dc7b and contender = 2216a0a. 2216a0a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.86% ⬆️0.09%] test-mac-arm
[Finished ⬇️1.63% ⬆️4.58%] ursa-i9-9960x
[Finished ⬇️0.52% ⬆️1.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2216a0a4 ec2-t3-xlarge-us-east-2
[Finished] 2216a0a4 test-mac-arm
[Finished] 2216a0a4 ursa-i9-9960x
[Finished] 2216a0a4 ursa-thinkcentre-m75q
[Finished] 584dc7bc ec2-t3-xlarge-us-east-2
[Finished] 584dc7bc test-mac-arm
[Finished] 584dc7bc ursa-i9-9960x
[Finished] 584dc7bc ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[felipecrv]

Rationale for this change

vector::at performs bounds checking and can throw an exception [1]. Its use is discouraged and in this specific case, the access is provably safe because array is previously resized to buffers.size().

[1] https://en.cppreference.com/w/cpp/container/vector/at

What changes are included in this PR?

Use of operator[] instead of at().

Are these changes tested?

By the existing concatenation tests.

[felipecrv]

@bkietz

[bkietz]

LGTM

[felipecrv]

Rebased and pushed again to see if all CI can pass now.

[pitrou]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

[mapleFU]

staticvoidVectorIndex(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec[i];
benchmark::DoNotOptimize(v);
}
}
}
// Register the function as a benchmarkBENCHMARK(VectorIndex);
staticvoidVectorAt(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec.at(i);
benchmark::DoNotOptimize(v);
}
}
}
BENCHMARK(VectorAt);

With release(-O2) on my MacOS:

------------------------------------------------------
Benchmark Time CPU Iterations
------------------------------------------------------
VectorIndex 24632 ns 17785 ns 38620
VectorAt 37438 ns 31141 ns 21829

[felipecrv]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

@pitrou C++ lore.

at() is forbidden in exception-free codebases which Arrow mostly is: Status and Result instead of throw everywhere.

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

[pitrou]

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

Ah, that's a good point.

[pitrou]

The "AMD64 Conda C++" CI failure seems unexpected, can you take a look?

[felipecrv]

@pitrou I only found unreleated issues, but one of them is being fixed in mapleFU's PR.

When that is merged I will rebase and force-push here.

[felipecrv]

I pushed a commit here to test a fix for parquet CI tests. If it passes, I will create a separate issue/PR for it.

[pitrou]

Can you rebase on git main so as to get a slightly less failing CI? :-)

[felipecrv]

Can you rebase on git main so as to get a slightly less failing CI? :-)

I'm waiting for @mapleFU's PR to be merged.

[pitrou]

CI failures are unrelated, will merge.

[ursabot]

Benchmark runs are scheduled for baseline = 584dc7b and contender = 2216a0a. 2216a0a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.86% ⬆️0.09%] test-mac-arm
[Finished ⬇️1.63% ⬆️4.58%] ursa-i9-9960x
[Finished ⬇️0.52% ⬆️1.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2216a0a4 ec2-t3-xlarge-us-east-2
[Finished] 2216a0a4 test-mac-arm
[Finished] 2216a0a4 ursa-i9-9960x
[Finished] 2216a0a4 ursa-thinkcentre-m75q
[Finished] 584dc7bc ec2-t3-xlarge-us-east-2
[Finished] 584dc7bc test-mac-arm
[Finished] 584dc7bc ursa-i9-9960x
[Finished] 584dc7bc ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[felipecrv]

Rationale for this change

vector::at performs bounds checking and can throw an exception [1]. Its use is discouraged and in this specific case, the access is provably safe because array is previously resized to buffers.size().

[1] https://en.cppreference.com/w/cpp/container/vector/at

What changes are included in this PR?

Use of operator[] instead of at().

Are these changes tested?

By the existing concatenation tests.

[felipecrv]

@bkietz

[bkietz]

LGTM

[felipecrv]

Rebased and pushed again to see if all CI can pass now.

[pitrou]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

[mapleFU]

staticvoidVectorIndex(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec[i];
benchmark::DoNotOptimize(v);
}
}
}
// Register the function as a benchmarkBENCHMARK(VectorIndex);
staticvoidVectorAt(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec.at(i);
benchmark::DoNotOptimize(v);
}
}
}
BENCHMARK(VectorAt);

With release(-O2) on my MacOS:

------------------------------------------------------
Benchmark Time CPU Iterations
------------------------------------------------------
VectorIndex 24632 ns 17785 ns 38620
VectorAt 37438 ns 31141 ns 21829

[felipecrv]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

@pitrou C++ lore.

at() is forbidden in exception-free codebases which Arrow mostly is: Status and Result instead of throw everywhere.

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

[pitrou]

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

Ah, that's a good point.

[pitrou]

The "AMD64 Conda C++" CI failure seems unexpected, can you take a look?

[felipecrv]

@pitrou I only found unreleated issues, but one of them is being fixed in mapleFU's PR.

When that is merged I will rebase and force-push here.

[felipecrv]

I pushed a commit here to test a fix for parquet CI tests. If it passes, I will create a separate issue/PR for it.

[pitrou]

Can you rebase on git main so as to get a slightly less failing CI? :-)

[felipecrv]

Can you rebase on git main so as to get a slightly less failing CI? :-)

I'm waiting for @mapleFU's PR to be merged.

[pitrou]

CI failures are unrelated, will merge.

[ursabot]

Benchmark runs are scheduled for baseline = 584dc7b and contender = 2216a0a. 2216a0a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.86% ⬆️0.09%] test-mac-arm
[Finished ⬇️1.63% ⬆️4.58%] ursa-i9-9960x
[Finished ⬇️0.52% ⬆️1.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2216a0a4 ec2-t3-xlarge-us-east-2
[Finished] 2216a0a4 test-mac-arm
[Finished] 2216a0a4 ursa-i9-9960x
[Finished] 2216a0a4 ursa-thinkcentre-m75q
[Finished] 584dc7bc ec2-t3-xlarge-us-east-2
[Finished] 584dc7bc test-mac-arm
[Finished] 584dc7bc ursa-i9-9960x
[Finished] 584dc7bc ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[felipecrv]

Rationale for this change

vector::at performs bounds checking and can throw an exception [1]. Its use is discouraged and in this specific case, the access is provably safe because array is previously resized to buffers.size().

[1] https://en.cppreference.com/w/cpp/container/vector/at

What changes are included in this PR?

Use of operator[] instead of at().

Are these changes tested?

By the existing concatenation tests.

[felipecrv]

@bkietz

[bkietz]

LGTM

[felipecrv]

Rebased and pushed again to see if all CI can pass now.

[pitrou]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

[mapleFU]

staticvoidVectorIndex(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec[i];
benchmark::DoNotOptimize(v);
}
}
}
// Register the function as a benchmarkBENCHMARK(VectorIndex);
staticvoidVectorAt(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec.at(i);
benchmark::DoNotOptimize(v);
}
}
}
BENCHMARK(VectorAt);

With release(-O2) on my MacOS:

------------------------------------------------------
Benchmark Time CPU Iterations
------------------------------------------------------
VectorIndex 24632 ns 17785 ns 38620
VectorAt 37438 ns 31141 ns 21829

[felipecrv]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

@pitrou C++ lore.

at() is forbidden in exception-free codebases which Arrow mostly is: Status and Result instead of throw everywhere.

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

[pitrou]

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

Ah, that's a good point.

[pitrou]

The "AMD64 Conda C++" CI failure seems unexpected, can you take a look?

[felipecrv]

@pitrou I only found unreleated issues, but one of them is being fixed in mapleFU's PR.

When that is merged I will rebase and force-push here.

[felipecrv]

I pushed a commit here to test a fix for parquet CI tests. If it passes, I will create a separate issue/PR for it.

[pitrou]

Can you rebase on git main so as to get a slightly less failing CI? :-)

[felipecrv]

Can you rebase on git main so as to get a slightly less failing CI? :-)

I'm waiting for @mapleFU's PR to be merged.

[pitrou]

CI failures are unrelated, will merge.

[ursabot]

Benchmark runs are scheduled for baseline = 584dc7b and contender = 2216a0a. 2216a0a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.86% ⬆️0.09%] test-mac-arm
[Finished ⬇️1.63% ⬆️4.58%] ursa-i9-9960x
[Finished ⬇️0.52% ⬆️1.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2216a0a4 ec2-t3-xlarge-us-east-2
[Finished] 2216a0a4 test-mac-arm
[Finished] 2216a0a4 ursa-i9-9960x
[Finished] 2216a0a4 ursa-thinkcentre-m75q
[Finished] 584dc7bc ec2-t3-xlarge-us-east-2
[Finished] 584dc7bc test-mac-arm
[Finished] 584dc7bc ursa-i9-9960x
[Finished] 584dc7bc ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[felipecrv]

Rationale for this change

vector::at performs bounds checking and can throw an exception [1]. Its use is discouraged and in this specific case, the access is provably safe because array is previously resized to buffers.size().

[1] https://en.cppreference.com/w/cpp/container/vector/at

What changes are included in this PR?

Use of operator[] instead of at().

Are these changes tested?

By the existing concatenation tests.

[felipecrv]

@bkietz

[bkietz]

LGTM

[felipecrv]

Rebased and pushed again to see if all CI can pass now.

[pitrou]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

[mapleFU]

staticvoidVectorIndex(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec[i];
benchmark::DoNotOptimize(v);
}
}
}
// Register the function as a benchmarkBENCHMARK(VectorIndex);
staticvoidVectorAt(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec.at(i);
benchmark::DoNotOptimize(v);
}
}
}
BENCHMARK(VectorAt);

With release(-O2) on my MacOS:

------------------------------------------------------
Benchmark Time CPU Iterations
------------------------------------------------------
VectorIndex 24632 ns 17785 ns 38620
VectorAt 37438 ns 31141 ns 21829

[felipecrv]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

@pitrou C++ lore.

at() is forbidden in exception-free codebases which Arrow mostly is: Status and Result instead of throw everywhere.

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

[pitrou]

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

Ah, that's a good point.

[pitrou]

The "AMD64 Conda C++" CI failure seems unexpected, can you take a look?

[felipecrv]

@pitrou I only found unreleated issues, but one of them is being fixed in mapleFU's PR.

When that is merged I will rebase and force-push here.

[felipecrv]

I pushed a commit here to test a fix for parquet CI tests. If it passes, I will create a separate issue/PR for it.

[pitrou]

Can you rebase on git main so as to get a slightly less failing CI? :-)

[felipecrv]

Can you rebase on git main so as to get a slightly less failing CI? :-)

I'm waiting for @mapleFU's PR to be merged.

[pitrou]

CI failures are unrelated, will merge.

[ursabot]

Benchmark runs are scheduled for baseline = 584dc7b and contender = 2216a0a. 2216a0a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.86% ⬆️0.09%] test-mac-arm
[Finished ⬇️1.63% ⬆️4.58%] ursa-i9-9960x
[Finished ⬇️0.52% ⬆️1.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2216a0a4 ec2-t3-xlarge-us-east-2
[Finished] 2216a0a4 test-mac-arm
[Finished] 2216a0a4 ursa-i9-9960x
[Finished] 2216a0a4 ursa-thinkcentre-m75q
[Finished] 584dc7bc ec2-t3-xlarge-us-east-2
[Finished] 584dc7bc test-mac-arm
[Finished] 584dc7bc ursa-i9-9960x
[Finished] 584dc7bc ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[felipecrv]

Rationale for this change

vector::at performs bounds checking and can throw an exception [1]. Its use is discouraged and in this specific case, the access is provably safe because array is previously resized to buffers.size().

[1] https://en.cppreference.com/w/cpp/container/vector/at

What changes are included in this PR?

Use of operator[] instead of at().

Are these changes tested?

By the existing concatenation tests.

[felipecrv]

@bkietz

[bkietz]

LGTM

[felipecrv]

Rebased and pushed again to see if all CI can pass now.

[pitrou]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

[mapleFU]

staticvoidVectorIndex(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec[i];
benchmark::DoNotOptimize(v);
}
}
}
// Register the function as a benchmarkBENCHMARK(VectorIndex);
staticvoidVectorAt(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec.at(i);
benchmark::DoNotOptimize(v);
}
}
}
BENCHMARK(VectorAt);

With release(-O2) on my MacOS:

------------------------------------------------------
Benchmark Time CPU Iterations
------------------------------------------------------
VectorIndex 24632 ns 17785 ns 38620
VectorAt 37438 ns 31141 ns 21829

[felipecrv]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

@pitrou C++ lore.

at() is forbidden in exception-free codebases which Arrow mostly is: Status and Result instead of throw everywhere.

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

[pitrou]

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

Ah, that's a good point.

[pitrou]

The "AMD64 Conda C++" CI failure seems unexpected, can you take a look?

[felipecrv]

@pitrou I only found unreleated issues, but one of them is being fixed in mapleFU's PR.

When that is merged I will rebase and force-push here.

[felipecrv]

I pushed a commit here to test a fix for parquet CI tests. If it passes, I will create a separate issue/PR for it.

[pitrou]

Can you rebase on git main so as to get a slightly less failing CI? :-)

[felipecrv]

Can you rebase on git main so as to get a slightly less failing CI? :-)

I'm waiting for @mapleFU's PR to be merged.

[pitrou]

CI failures are unrelated, will merge.

[ursabot]

Benchmark runs are scheduled for baseline = 584dc7b and contender = 2216a0a. 2216a0a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.86% ⬆️0.09%] test-mac-arm
[Finished ⬇️1.63% ⬆️4.58%] ursa-i9-9960x
[Finished ⬇️0.52% ⬆️1.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2216a0a4 ec2-t3-xlarge-us-east-2
[Finished] 2216a0a4 test-mac-arm
[Finished] 2216a0a4 ursa-i9-9960x
[Finished] 2216a0a4 ursa-thinkcentre-m75q
[Finished] 584dc7bc ec2-t3-xlarge-us-east-2
[Finished] 584dc7bc test-mac-arm
[Finished] 584dc7bc ursa-i9-9960x
[Finished] 584dc7bc ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[felipecrv]

Rationale for this change

vector::at performs bounds checking and can throw an exception [1]. Its use is discouraged and in this specific case, the access is provably safe because array is previously resized to buffers.size().

[1] https://en.cppreference.com/w/cpp/container/vector/at

What changes are included in this PR?

Use of operator[] instead of at().

Are these changes tested?

By the existing concatenation tests.

[felipecrv]

@bkietz

[bkietz]

LGTM

[felipecrv]

Rebased and pushed again to see if all CI can pass now.

[pitrou]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

[mapleFU]

staticvoidVectorIndex(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec[i];
benchmark::DoNotOptimize(v);
}
}
}
// Register the function as a benchmarkBENCHMARK(VectorIndex);
staticvoidVectorAt(benchmark::State& state) {
// Code inside this loop is measured repeatedly
std::vector<int64_t> range_vec(10000, 0);
for (auto _ : state) {
// Make sure the variable is not optimized away by compilerfor (int i = 0; i < 10000; ++i) {
int64_t v = range_vec.at(i);
benchmark::DoNotOptimize(v);
}
}
}
BENCHMARK(VectorAt);

With release(-O2) on my MacOS:

------------------------------------------------------
Benchmark Time CPU Iterations
------------------------------------------------------
VectorIndex 24632 ns 17785 ns 38620
VectorAt 37438 ns 31141 ns 21829

[felipecrv]

This PR is ok but I'm not sure std::vector::at is "discouraged". Can you provide a reference to that claim?

@pitrou C++ lore.

at() is forbidden in exception-free codebases which Arrow mostly is: Status and Result instead of throw everywhere.

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

[pitrou]

We return a bad Status when we bounds-check and try to avoid bounds-checking when we can prove that access is safe. That's why I believe it's fair to say the use of at is discouraged — Arrow's bounds-checks are explicit, non-superfluous and lead to bad Status instead of an exception being thrown.

Ah, that's a good point.

[pitrou]

The "AMD64 Conda C++" CI failure seems unexpected, can you take a look?

[felipecrv]

@pitrou I only found unreleated issues, but one of them is being fixed in mapleFU's PR.

When that is merged I will rebase and force-push here.

[felipecrv]

I pushed a commit here to test a fix for parquet CI tests. If it passes, I will create a separate issue/PR for it.

[pitrou]

Can you rebase on git main so as to get a slightly less failing CI? :-)

[felipecrv]

Can you rebase on git main so as to get a slightly less failing CI? :-)

I'm waiting for @mapleFU's PR to be merged.

[pitrou]

CI failures are unrelated, will merge.

[ursabot]

Benchmark runs are scheduled for baseline = 584dc7b and contender = 2216a0a. 2216a0a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.86% ⬆️0.09%] test-mac-arm
[Finished ⬇️1.63% ⬆️4.58%] ursa-i9-9960x
[Finished ⬇️0.52% ⬆️1.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 2216a0a4 ec2-t3-xlarge-us-east-2
[Finished] 2216a0a4 test-mac-arm
[Finished] 2216a0a4 ursa-i9-9960x
[Finished] 2216a0a4 ursa-thinkcentre-m75q
[Finished] 584dc7bc ec2-t3-xlarge-us-east-2
[Finished] 584dc7bc test-mac-arm
[Finished] 584dc7bc ursa-i9-9960x
[Finished] 584dc7bc ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java