You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mapleFU (via GitHub)" <gi...@apache.org> on 2023/05/16 03:07:45 UTC
[GitHub] [arrow] mapleFU opened a new pull request, #35605: GH-35571: [C++][CI][Parquet] adding comparing logs in Decryption tests
mapleFU opened a new pull request, #35605:
URL: https://github.com/apache/arrow/pull/35605
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
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}
-->
### Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
CI would failed in Decrypt test in Windows MinGW.
### What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Currently add some logs to debug
### Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
No
### Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
--
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 #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35605:
URL: https://github.com/apache/arrow/pull/35605#discussion_r1196430612
##########
cpp/src/parquet/encryption/test_encryption_util.cc:
##########
@@ -322,9 +322,20 @@ void ReadAndVerifyColumn(RowGroupReader* rg_reader, RowGroupMetadata* rg_md,
}
ASSERT_EQ(rows_read, rows_should_read);
ASSERT_EQ(values_read, rows_should_read);
- ASSERT_EQ(read_col_data.values, expected_column_data.values);
// make sure we got the same number of values the metadata says
ASSERT_EQ(col_md->num_values(), rows_read);
+ if constexpr (std::is_floating_point_v<typename DType::c_type>) {
Review Comment:
```suggestion
// GH-35571: need to use approximate floating-point comparison because of
// precision issues on MinGW32 (the values generated in the C++ test code
// may not exactly match those from the parquet-testing data files).
if constexpr (std::is_floating_point_v<typename DType::c_type>) {
```
--
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] mapleFU commented on pull request #35605: GH-35571: [C++][CI][Parquet] adding comparing logs in Decryption tests
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549078718
Another problem occured when casting decimal to string:
```
[----------] 2 tests from TestDecimalFromReal/1, where TypeParam = std::pair<arrow::Decimal128, double>
[ RUN ] TestDecimalFromReal/1.TestSuccess
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "4611686000000000000"
expected
Which is: "4611686018427387904"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "-4611686000000000000"
expected
Which is: "-4611686018427387904"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "9223372000000000000"
expected
Which is: "9223372036854775808"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "-9223372000000000000"
expected
Which is: "-9223372036854775808"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "18446744000000000000"
expected
Which is: "18446744073709551616"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "-18446744000000000000"
expected
Which is: "-18446744073709551616"
[ FAILED ] TestDecimalFromReal/1.TestSuccess, where TypeParam = std::pair<arrow::Decimal128, double> (0 ms)
[ RUN ] TestDecimalFromReal/1.TestErrors
[ OK ] TestDecimalFromReal/1.TestErrors (0 ms)
[----------] 2 tests from TestDecimalFromReal/1 (0 ms total)
[----------] 2 tests from TestDecimalFromReal/2, where TypeParam = std::pair<arrow::Decimal256, float>
[ RUN ] TestDecimalFromReal/2.TestSuccess
[ OK ] TestDecimalFromReal/2.TestSuccess (0 ms)
[ RUN ] TestDecimalFromReal/2.TestErrors
[ OK ] TestDecimalFromReal/2.TestErrors (0 ms)
[----------] 2 tests from TestDecimalFromReal/2 (0 ms total)
[----------] 2 tests from TestDecimalFromReal/3, where TypeParam = std::pair<arrow::Decimal256, double>
[ RUN ] TestDecimalFromReal/3.TestSuccess
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "4611686000000000000"
expected
Which is: "4611686018427387904"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "-4611686000000000000"
expected
Which is: "-4611686018427387904"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "9223372000000000000"
expected
Which is: "9223372036854775808"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "-9223372000000000000"
expected
Which is: "-9223372036854775808"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "18446744000000000000"
expected
Which is: "18446744073709551616"
D:/a/arrow/arrow/cpp/src/arrow/util/decimal_test.cc:766: Failure
Expected equality of these values:
dec.ToString(scale)
Which is: "-18446744000000000000"
expected
Which is: "-18446744073709551616"
[ FAILED ] TestDecimalFromReal/3.TestSuccess, where TypeParam = std::pair<arrow::Decimal256, double> (0 ms)
[ RUN ] TestDecimalFromReal/3.TestErrors
```
I gothrough the related code.
--
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] mapleFU commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549841604
I guess MinGW with unstable CI may generate or truncate data when getting `GenerateSampleData`, so I guess previous fixing in https://github.com/apache/arrow/pull/35605/commits/6265cc17f8b35501f793d3f3c25a972e2ecd2841 is ok
@pitrou What would you think of 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
[GitHub] [arrow] pitrou commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549977184
> https://github.com/apache/parquet-testing/tree/master/data The data is already here, I guess maybe it cannot help?
Ahaha! Well, that explains why there might be a discrepancy then...
--
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 #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1550001231
:warning: GitHub issue #35571 **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] github-actions[bot] commented on pull request #35605: GH-35571: [C++][CI][Parquet] adding comparing logs in Decryption tests
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1548910736
:warning: GitHub issue #35571 **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 pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549671379
Rather than complicating the test routine, I'd rather generate data that doesn't trigger truncation issues (wherever they might happen):
```diff
diff --git a/cpp/src/parquet/encryption/test_encryption_util.cc b/cpp/src/parquet/encryption/test_encryption_util.cc
index 012712c7b..8193e6525 100644
--- a/cpp/src/parquet/encryption/test_encryption_util.cc
+++ b/cpp/src/parquet/encryption/test_encryption_util.cc
@@ -143,7 +143,7 @@ template <>
ColumnData<FloatType> GenerateSampleData<FloatType>(int rows) {
ColumnData<FloatType> float_col;
for (int i = 0; i < rows; i++) {
- float value = static_cast<float>(i) * 1.1f;
+ float value = static_cast<float>(i) * 1.5f;
float_col.values.push_back(value);
}
return float_col;
```
--
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 #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35605:
URL: https://github.com/apache/arrow/pull/35605#discussion_r1195415260
##########
cpp/src/parquet/encryption/test_encryption_util.cc:
##########
@@ -322,9 +322,24 @@ void ReadAndVerifyColumn(RowGroupReader* rg_reader, RowGroupMetadata* rg_md,
}
ASSERT_EQ(rows_read, rows_should_read);
ASSERT_EQ(values_read, rows_should_read);
- ASSERT_EQ(read_col_data.values, expected_column_data.values);
- // make sure we got the same number of values the metadata says
- ASSERT_EQ(col_md->num_values(), rows_read);
+ if constexpr (std::is_floating_point_v<typename DType::c_type>) {
+ ASSERT_EQ(read_col_data.rows(), expected_column_data.rows());
+ for (int i = 0; i < read_col_data.rows(); ++i) {
+ if (std::isnan(expected_column_data.values[i])) {
+ EXPECT_TRUE(std::isnan(read_col_data.values[i]))
+ << "Values at index " << i << " is not nan";
+ continue;
+ }
+ if constexpr (std::is_same_v<float, typename DType::c_type>) {
+ EXPECT_FLOAT_EQ(expected_column_data.values[i], read_col_data.values[i]);
+ } else {
+ EXPECT_DOUBLE_EQ(expected_column_data.values[i], read_col_data.values[i]);
+ }
+ }
+ } else {
+ // make sure we got the same number of values the metadata says
+ ASSERT_EQ(col_md->num_values(), rows_read);
Review Comment:
This should be done always, not only for non-floating-point values.
Also, you still need to compare values for non-floating-point.
##########
cpp/src/parquet/encryption/test_encryption_util.cc:
##########
@@ -322,9 +322,24 @@ void ReadAndVerifyColumn(RowGroupReader* rg_reader, RowGroupMetadata* rg_md,
}
ASSERT_EQ(rows_read, rows_should_read);
ASSERT_EQ(values_read, rows_should_read);
- ASSERT_EQ(read_col_data.values, expected_column_data.values);
- // make sure we got the same number of values the metadata says
- ASSERT_EQ(col_md->num_values(), rows_read);
+ if constexpr (std::is_floating_point_v<typename DType::c_type>) {
+ ASSERT_EQ(read_col_data.rows(), expected_column_data.rows());
+ for (int i = 0; i < read_col_data.rows(); ++i) {
+ if (std::isnan(expected_column_data.values[i])) {
Review Comment:
There doesn't seem to be any NaN in the generated data, this can be removed.
--
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 #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549084635
:warning: GitHub issue #35571 **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] mapleFU commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1550586577
Comment fixed. The case here ( https://github.com/apache/arrow/pull/35605#issuecomment-1549078718 ) need to be fixed later, so "C++ / AMD64 Windows MinGW MINGW64 C++" still failed
--
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] ursabot commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1555430298
Benchmark runs are scheduled for baseline = 6f0b34897b8645643493ce8907e4e441b274be3e and contender = 37975d52e7bf829f9e5a23a65d584523d7616a4d. 37975d52e7bf829f9e5a23a65d584523d7616a4d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/7595592c5c244e2a8efcdd043365b0c7...3a8dfd4d8b6a484b84771fb05199326d/)
[Finished :arrow_down:0.42% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/28424ac135ad4b62bdc402c322cd1baa...e846476bb10c4fd1a97872c0c3f186d3/)
[Finished :arrow_down:1.31% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f0e0fdb854c247c3a557439fad57744e...45470e88d00e4ddc81e765f986ae2601/)
[Finished :arrow_down:0.15% :arrow_up:0.03%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/23cc5ca9c22942a9adf87b98e886003a...6aab720dc3214cf488d2cf3d09c2ec4f/)
Buildkite builds:
[Finished] [`37975d52` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2889)
[Finished] [`37975d52` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2925)
[Finished] [`37975d52` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2890)
[Finished] [`37975d52` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2915)
[Finished] [`6f0b3489` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2888)
[Finished] [`6f0b3489` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2924)
[Finished] [`6f0b3489` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2889)
[Finished] [`6f0b3489` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2914)
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
--
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] mapleFU commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549302360
Got it, I'm at office and didn't have a Windows machine, I'll try to reproduce it later after I'm home
--
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] mapleFU commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549243172
I guess the error and https://github.com/apache/arrow/issues/35606 have same underlying reason. Maybe there is some prevision loss in MinGW
--
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 #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549276994
There shouldn't be any precision loss simply when copying values. Perhaps the tests are doing something wrong.
--
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] ursabot commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1555432339
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f0e0fdb854c247c3a557439fad57744e...45470e88d00e4ddc81e765f986ae2601/)
--
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 #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #35605:
URL: https://github.com/apache/arrow/pull/35605
--
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 #35605: GH-35571: [C++][CI][Parquet] adding comparing logs in Decryption tests
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1548910719
* Closes: #35571
--
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] mapleFU commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549801882
Apply pitrou's idea, waiting for ci and see what would happen
--
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 #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549232052
```
values at index 28 not equal read is 30.800001, expect 30.799999
```
This is not right. Reading Parquet values should be bit-precise, not approximate.
--
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] felipecrv commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549625441
> There shouldn't be any precision loss simply when copying values. Perhaps the tests are doing something wrong.
Could this be tracked in a separate issue and the CI fix merged sooner rather than later?
--
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] mapleFU commented on pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1550586619
Comment fixed. The case here ( https://github.com/apache/arrow/pull/35605#issuecomment-1549078718 ) need to be fixed later, so "C++ / AMD64 Windows MinGW MINGW64 C++" still failed
--
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 #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549084325
:warning: GitHub issue #35571 **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] mapleFU commented on pull request #35605: GH-35571: [C++][CI][Parquet] adding comparing logs in Decryption tests
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1549083386
@pitrou @wjones127 Would you mind take a look at this 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
[GitHub] [arrow] mapleFU commented on pull request #35605: GH-35571: [C++][CI][Parquet] adding comparing logs in Decryption tests
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #35605:
URL: https://github.com/apache/arrow/pull/35605#issuecomment-1548972064
```
D:/a/arrow/arrow/cpp/src/parquet/encryption/test_encryption_util.cc:343: Failure
Expected equality of these values:
expected_column_data.values[i]
Which is: 19.8
read_col_data.values[i]
Which is: 19.8
values at index 18 not equal read is 19.800001, expect 19.799999
D:/a/arrow/arrow/cpp/src/parquet/encryption/test_encryption_util.cc:343: Failure
Expected equality of these values:
expected_column_data.values[i]
Which is: 25.3
read_col_data.values[i]
Which is: 25.3
values at index 23 not equal read is 25.300001, expect 25.299999
D:/a/arrow/arrow/cpp/src/parquet/encryption/test_encryption_util.cc:343: Failure
Expected equality of these values:
expected_column_data.values[i]
Which is: 26.4
read_col_data.values[i]
Which is: 26.4
values at index 24 not equal read is 26.400002, expect 26.400000
D:/a/arrow/arrow/cpp/src/parquet/encryption/test_encryption_util.cc:343: Failure
Expected equality of these values:
expected_column_data.values[i]
Which is: 30.8
read_col_data.values[i]
Which is: 30.8
values at index 28 not equal read is 30.800001, expect 30.799999
```
I guess the problem comes from floating point equality. @felipecrv
--
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