Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-8593: [C++][Parquet] Fix build with musl libc #7038

Closed
wants to merge 1 commit into from

Conversation

tobim
Copy link
Contributor

@tobim tobim commented Apr 25, 2020

Converts local constants in file_serialize_test.cc to snake_case.

Fixes a confilct with the PAGE_SIZE macro declared in the limits.h header that is shipped with musl libc.

@github-actions
Copy link

@wesm
Copy link
Member

wesm commented Apr 27, 2020

@kszucs would this be caught in a rehabilitated Alpine nightly build?

@@ -346,12 +346,12 @@ TEST(TestBufferedRowGroupWriter, DisabledDictionary) {
}

TEST(TestBufferedRowGroupWriter, MultiPageDisabledDictionary) {
const int VALUE_COUNT = 10000;
const int PAGE_SIZE = 16384;
const int value_count = 10000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit as long as you are changing this, these should probably be constexpr kValueCount, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And moved to constexpr while were at it.

@fsaintjacques
Copy link
Contributor

@tobim, I do not have the rights to push-force on this branch. You can apply this locally:

diff --git a/cpp/src/parquet/file_serialize_test.cc b/cpp/src/parquet/file_serialize_test.cc
index ec6976715f..ac422a76c7 100644
--- a/cpp/src/parquet/file_serialize_test.cc
+++ b/cpp/src/parquet/file_serialize_test.cc
@@ -346,12 +346,12 @@ TEST(TestBufferedRowGroupWriter, DisabledDictionary) {
 }
 
 TEST(TestBufferedRowGroupWriter, MultiPageDisabledDictionary) {
-  const int VALUE_COUNT = 10000;
-  const int PAGE_SIZE = 16384;
+  constexpr int kValueCount = 10000;
+  constexpr int kPageSize = 16384;
   auto sink = CreateOutputStream();
   auto writer_props = parquet::WriterProperties::Builder()
                           .disable_dictionary()
-                          ->data_pagesize(PAGE_SIZE)
+                          ->data_pagesize(kPageSize)
                           ->build();
   schema::NodeVector fields;
   fields.push_back(
@@ -362,10 +362,10 @@ TEST(TestBufferedRowGroupWriter, MultiPageDisabledDictionary) {
   auto rg_writer = file_writer->AppendBufferedRowGroup();
   auto col_writer = static_cast<Int32Writer*>(rg_writer->column(0));
   std::vector<int32_t> values_in;
-  for (int i = 0; i < VALUE_COUNT; ++i) {
+  for (int i = 0; i < kValueCount; ++i) {
     values_in.push_back((i % 100) + 1);
   }
-  col_writer->WriteBatch(VALUE_COUNT, nullptr, nullptr, values_in.data());
+  col_writer->WriteBatch(kValueCount, nullptr, nullptr, values_in.data());
   rg_writer->Close();
   file_writer->Close();
   PARQUET_ASSIGN_OR_THROW(auto buffer, sink->Finish());
@@ -374,17 +374,17 @@ TEST(TestBufferedRowGroupWriter, MultiPageDisabledDictionary) {
   auto file_reader = ParquetFileReader::Open(source);
   auto file_metadata = file_reader->metadata();
   ASSERT_EQ(1, file_reader->metadata()->num_row_groups());
-  std::vector<int32_t> values_out(VALUE_COUNT);
+  std::vector<int32_t> values_out(kValueCount);
   for (int r = 0; r < file_metadata->num_row_groups(); ++r) {
     auto rg_reader = file_reader->RowGroup(r);
     ASSERT_EQ(1, rg_reader->metadata()->num_columns());
-    ASSERT_EQ(VALUE_COUNT, rg_reader->metadata()->num_rows());
+    ASSERT_EQ(kValueCount, rg_reader->metadata()->num_rows());
     int64_t total_values_read = 0;
     std::shared_ptr<parquet::ColumnReader> col_reader;
     ASSERT_NO_THROW(col_reader = rg_reader->Column(0));
     parquet::Int32Reader* int32_reader =
         static_cast<parquet::Int32Reader*>(col_reader.get());
-    int64_t vn = VALUE_COUNT;
+    int64_t vn = kValueCount;
     int32_t* vx = values_out.data();
     while (int32_reader->HasNext()) {
       int64_t values_read;
@@ -393,7 +393,7 @@ TEST(TestBufferedRowGroupWriter, MultiPageDisabledDictionary) {
       vx += values_read;
       total_values_read += values_read;
     }
-    ASSERT_EQ(VALUE_COUNT, total_values_read);
+    ASSERT_EQ(kValueCount, total_values_read);
     ASSERT_EQ(values_in, values_out);
   }
 }

@tobim tobim force-pushed the ARROW-8593/fix-parquet-test-musl branch from 00b34ab to 176bc03 Compare May 1, 2020 19:39
Thereby fixing a confilct with the `PAGE_SIZE` macro declared in
the `limits.h` header that is shipped with musl libc.
@tobim tobim force-pushed the ARROW-8593/fix-parquet-test-musl branch from 176bc03 to 1cdc618 Compare May 1, 2020 19:42
@tobim
Copy link
Contributor Author

tobim commented May 1, 2020

@fsaintjacques @emkornfield sorry for the long silence, I updated the commit as you suggested.

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants