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/05/16 16:27:13 UTC
[GitHub] [arrow] pitrou commented on a diff in pull request #35605: GH-35571: [C++][CI][Parquet] Change `EQ` to `FLOAT_EQ` in Decryption tests
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