You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2023/07/19 08:57:25 UTC
[GitHub] [arrow] pitrou opened a new pull request, #36769: GH-36767: [C++][CI] Fix test failure on i386
pitrou opened a new pull request, #36769:
URL: https://github.com/apache/arrow/pull/36769
### Rationale for this change
Our nightly build on i386 Debian (a 32-bit x86 build) fails with an error in decimal-to-read tests:
https://github.com/ursacomputing/crossbow/actions/runs/5593823296/jobs/10227952675#step:6:3255
### What changes are included in this PR?
Improve error messages by displaying the actual and expected values.
A side effect of this error message improvement is to... fix the test failure, as storing the computation result in a local variable seems to change the computed absolute difference.
This is probably due to x86 FPU rounding shenanigans, as explained here: https://stackoverflow.com/questions/37626687/c-fundamentals-double-variable-not-equal-to-double-expression
### Are these changes tested?
Yes.
### Are there any user-facing changes?
No.
--
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
[GitHub] [arrow] pitrou merged pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #36769:
URL: https://github.com/apache/arrow/pull/36769
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36769:
URL: https://github.com/apache/arrow/pull/36769#issuecomment-1641728564
Revision: a17acd4dc591928e9bea75b0afbfa0b92ca4d0cb
Submitted crossbow builds: [ursacomputing/crossbow @ actions-7d00f83584](https://github.com/ursacomputing/crossbow/branches/all?query=actions-7d00f83584)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597282395/jobs/10235425446)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/5597282750/jobs/10235426181)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597278916/jobs/10235417428)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/15161225529)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597278349/jobs/10235416237)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5597279143/jobs/10235417976)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/5597282061/jobs/10235424716)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597280954/jobs/10235422166)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597281685/jobs/10235423893)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/5597278583/jobs/10235416680)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/5597280069/jobs/10235420326)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/5597280667/jobs/10235421517)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597280435/jobs/10235421013)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/5597279431/jobs/10235418835)|
--
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
[GitHub] [arrow] pitrou commented on a diff in pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36769:
URL: https://github.com/apache/arrow/pull/36769#discussion_r1267791880
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -1046,42 +1046,51 @@ using ToDoubleTestParam = ToRealTestParam<double>;
template <typename Decimal, typename Real>
void CheckDecimalToReal(const std::string& decimal_value, int32_t scale, Real expected) {
Decimal dec(decimal_value);
- ASSERT_EQ(dec.template ToReal<Real>(scale), expected)
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
Review Comment:
I wonder if that's necessary. Introducing a deliberate error seems to display the values with sufficient precision:
```
[ RUN ] TestDecimalToRealFloat/0.Precision
/arrow/cpp/src/arrow/util/decimal_test.cc:1071: Failure
Value of: std::abs(actual - expected) <= epsilon
Actual: false
Expected: true
Decimal value: 1.987748987892758765582589910934859345, scale: 36, expected: 1.9877489805221558, actual: 1.9877490997314453
[ FAILED ] TestDecimalToRealFloat/0.Precision, where TypeParam = arrow::Decimal128 (0 ms)
```
--
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
[GitHub] [arrow] js8544 commented on a diff in pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36769:
URL: https://github.com/apache/arrow/pull/36769#discussion_r1267801800
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -1046,42 +1046,51 @@ using ToDoubleTestParam = ToRealTestParam<double>;
template <typename Decimal, typename Real>
void CheckDecimalToReal(const std::string& decimal_value, int32_t scale, Real expected) {
Decimal dec(decimal_value);
- ASSERT_EQ(dec.template ToReal<Real>(scale), expected)
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
Review Comment:
I see. It looks ok to me!
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36769:
URL: https://github.com/apache/arrow/pull/36769#issuecomment-1641703553
Revision: c7de4d9b0075bda4cc46463b904f82107f1c5119
Submitted crossbow builds: [ursacomputing/crossbow @ actions-584b3d34e9](https://github.com/ursacomputing/crossbow/branches/all?query=actions-584b3d34e9)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597112364/jobs/10235034878)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/5597114091/jobs/10235038984)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597113164/jobs/10235036805)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/15160732462)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597113408/jobs/10235037425)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5597111299/jobs/10235032098)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/5597112883/jobs/10235036067)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597110471/jobs/10235030340)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597112056/jobs/10235034130)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/5597110974/jobs/10235031470)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/5597111596/jobs/10235032874)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/5597113704/jobs/10235038068)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5597111854/jobs/10235033540)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/5597114454/jobs/10235039997)|
--
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
[GitHub] [arrow] pitrou commented on pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36769:
URL: https://github.com/apache/arrow/pull/36769#issuecomment-1641724208
@github-actions crossbow submit -g cpp
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36769:
URL: https://github.com/apache/arrow/pull/36769#issuecomment-1641699043
:warning: GitHub issue #36767 **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
[GitHub] [arrow] pitrou commented on a diff in pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36769:
URL: https://github.com/apache/arrow/pull/36769#discussion_r1267788354
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -1046,42 +1046,51 @@ using ToDoubleTestParam = ToRealTestParam<double>;
template <typename Decimal, typename Real>
void CheckDecimalToReal(const std::string& decimal_value, int32_t scale, Real expected) {
Decimal dec(decimal_value);
- ASSERT_EQ(dec.template ToReal<Real>(scale), expected)
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
+ ASSERT_EQ(actual, expected) << "Decimal value: " << decimal_value
+ << ", scale: " << scale << ", expected: " << expected
+ << ", actual: " << actual;
}
template <typename Decimal, typename Real>
void CheckDecimalToRealWithinOneULP(const std::string& decimal_value, int32_t scale,
Real expected) {
Decimal dec(decimal_value);
- auto result = dec.template ToReal<Real>(scale);
- ASSERT_TRUE(result == expected || result == std::nextafter(expected, expected + 1) ||
- result == std::nextafter(expected, expected - 1))
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
+ ASSERT_TRUE(actual == expected || actual == std::nextafter(expected, expected + 1) ||
+ actual == std::nextafter(expected, expected - 1))
+ << "Decimal value: " << decimal_value << ", scale: " << scale
+ << ", expected: " << expected << ", actual: " << actual;
}
template <typename Decimal, typename Real>
void CheckDecimalToRealWithinEpsilon(const std::string& decimal_value, int32_t scale,
Real epsilon, Real expected) {
Decimal dec(decimal_value);
- ASSERT_TRUE(std::abs(dec.template ToReal<Real>(scale) - expected) <= epsilon)
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
+ ASSERT_TRUE(std::abs(actual - expected) <= epsilon)
Review Comment:
Ah, right. I wonder if it reintroduces the failure :-D
--
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
[GitHub] [arrow] pitrou commented on pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36769:
URL: https://github.com/apache/arrow/pull/36769#issuecomment-1641698890
@github-actions crossbow submit -g cpp
--
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
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36769:
URL: https://github.com/apache/arrow/pull/36769#issuecomment-1655068227
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit be2014a9ebfb9570b016a5f0beda11022ace45d1.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/15419023859) has more details. It also includes information about 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
[GitHub] [arrow] js8544 commented on a diff in pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36769:
URL: https://github.com/apache/arrow/pull/36769#discussion_r1267774111
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -1046,42 +1046,51 @@ using ToDoubleTestParam = ToRealTestParam<double>;
template <typename Decimal, typename Real>
void CheckDecimalToReal(const std::string& decimal_value, int32_t scale, Real expected) {
Decimal dec(decimal_value);
- ASSERT_EQ(dec.template ToReal<Real>(scale), expected)
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
+ ASSERT_EQ(actual, expected) << "Decimal value: " << decimal_value
+ << ", scale: " << scale << ", expected: " << expected
+ << ", actual: " << actual;
}
template <typename Decimal, typename Real>
void CheckDecimalToRealWithinOneULP(const std::string& decimal_value, int32_t scale,
Real expected) {
Decimal dec(decimal_value);
- auto result = dec.template ToReal<Real>(scale);
- ASSERT_TRUE(result == expected || result == std::nextafter(expected, expected + 1) ||
- result == std::nextafter(expected, expected - 1))
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
+ ASSERT_TRUE(actual == expected || actual == std::nextafter(expected, expected + 1) ||
+ actual == std::nextafter(expected, expected - 1))
+ << "Decimal value: " << decimal_value << ", scale: " << scale
+ << ", expected: " << expected << ", actual: " << actual;
}
template <typename Decimal, typename Real>
void CheckDecimalToRealWithinEpsilon(const std::string& decimal_value, int32_t scale,
Real epsilon, Real expected) {
Decimal dec(decimal_value);
- ASSERT_TRUE(std::abs(dec.template ToReal<Real>(scale) - expected) <= epsilon)
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
+ ASSERT_TRUE(std::abs(actual - expected) <= epsilon)
Review Comment:
Maybe change it to ASSERT_LE(std::abs(actual - expected), epsilon) for an "even better" message?
```suggestion
ASSERT_LE(std::abs(actual - expected), epsilon)
```
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -1046,42 +1046,51 @@ using ToDoubleTestParam = ToRealTestParam<double>;
template <typename Decimal, typename Real>
void CheckDecimalToReal(const std::string& decimal_value, int32_t scale, Real expected) {
Decimal dec(decimal_value);
- ASSERT_EQ(dec.template ToReal<Real>(scale), expected)
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
Review Comment:
I'm curious: is it possible to set the precision for gtest outputs like `std::setprecision`. The default precision won't be very helpful for small diffs.
--
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
[GitHub] [arrow] js8544 commented on a diff in pull request #36769: GH-36767: [C++][CI] Fix test failure on i386
Posted by "js8544 (via GitHub)" <gi...@apache.org>.
js8544 commented on code in PR #36769:
URL: https://github.com/apache/arrow/pull/36769#discussion_r1267786109
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -1046,42 +1046,51 @@ using ToDoubleTestParam = ToRealTestParam<double>;
template <typename Decimal, typename Real>
void CheckDecimalToReal(const std::string& decimal_value, int32_t scale, Real expected) {
Decimal dec(decimal_value);
- ASSERT_EQ(dec.template ToReal<Real>(scale), expected)
- << "Decimal value: " << decimal_value << " Scale: " << scale;
+ Real actual = dec.template ToReal<Real>(scale);
Review Comment:
I'm curious: is it possible to set the precision for gtest outputs like `std::setprecision`? The default precision won't be very helpful for small diffs.
--
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