PARQUET-679: Local Windows build and Appveyor support #313

maxhora · 2017-04-26T22:57:37Z

No description provided.

majetideepak · 2017-04-26T23:53:10Z

CMakeLists.txt

@@ -661,11 +669,16 @@ if (PARQUET_BUILD_SHARED)
 endif()

 if (PARQUET_BUILD_STATIC)
+    if (MSVC)
+      set(LIB_NAME_STATIC parquet_static)


use PARQUET_LIB_NAME instead?

wesm · 2017-04-27T02:14:36Z

I asked ASF Infra to turn on Appveyor https://issues.apache.org/jira/browse/INFRA-14020

wesm

awesome! minor comments and some questions about licensing re: snappy build files

wesm · 2017-04-27T02:16:18Z

CMakeLists.txt

    add_library(parquet_static STATIC $<TARGET_OBJECTS:parquet_objlib>)
    set_target_properties(parquet_static
      PROPERTIES
      LIBRARY_OUTPUT_DIRECTORY "${BUILD_OUTPUT_ROOT_DIRECTORY}"
-      OUTPUT_NAME "parquet")
+      OUTPUT_NAME ${LIB_NAME_STATIC})
    target_link_libraries(parquet_static
      LINK_PUBLIC ${LIBPARQUET_INTERFACE_LINK_LIBS})
 endif()


can you adapt the Arrow function for doing this, maybe a little more reusable / readable? https://github.com/apache/arrow/blob/master/cpp/cmake_modules/BuildUtils.cmake#L85

wesm · 2017-04-27T02:16:41Z

README.md

@@ -265,3 +269,5 @@ coveralls -t $PARQUET_CPP_COVERAGE_TOKEN --gcov-options '\-l' -r $PARQUET_ROOT -

 Note that `gcov` throws off artifacts from the STL, so I excluded my toolchain
 root stored in `$NATIVE_TOOLCHAIN` to avoid a cluttered coverage report.
+
+[1]: https://github.com/apache/parquet-cpp/blob/master/Windows.md


Create a docs directory for this?

wesm · 2017-04-27T02:18:34Z

Windows.md

+
+## Fast setup of building requirements with conda and conda-forge
+
+To not waste a lot of time to setup the thirdparty build dependecies for


Some might disagree that it's a waste of time, but it's indeed convenient, so "A convenient and tested way to set up thirdparty dependencies is to use the conda package manager."

wesm · 2017-04-27T02:19:04Z

Windows.md

+
+```shell
+conda config --add channels conda-forge
+```


Instead of using conda config, pass -c conda-forge to conda create

wesm · 2017-04-27T02:19:39Z

Windows.md

+Now, you can bootstrap a build environment
+
+```shell
+conda create -n parquet-dev cmake git boost curl zlib


If you install boost-cpp instead of boost, it will not pull in a separate Python install. Do we also need Snappy here?

Snappy is required, but we don't have Windows build yet in conda-forge

Is Snappy for Windows available in defaults (repo.continuum.io)? Support I could look myself =)

You're right, it's available in defaults, going to add it too, thank you!

wesm · 2017-04-27T02:25:57Z

src/parquet/encoding-internal.h

@@ -852,8 +852,8 @@ class DeltaLengthByteArrayDecoder : public Decoder<ByteArrayType> {

  virtual int Decode(ByteArray* buffer, int max_values) {
    max_values = std::min(max_values, num_values_);
-    int lengths[max_values];
-    len_decoder_.Decode(lengths, max_values);
+    std::unique_ptr<int[]> lengths(new int[max_values]);


Use std::vector here

wesm · 2017-04-27T02:27:01Z

src/parquet/util/cpu-info.h

@@ -73,7 +75,7 @@ class CpuInfo {
  /// Returns the model name of the cpu (e.g. Intel i7-2600)
  static std::string model_name();

-  static bool initialized() { return initialized_; }
+  static bool initialized();


It's better to inline these if possible

wesm · 2017-04-27T02:27:42Z

src/parquet/util/rle-test.cc

  const uint64_t mod = bit_width == 64 ? 1 : 1LL << bit_width;

-  uint8_t buffer[len];
-  BitWriter writer(buffer, len);
+  std::unique_ptr<uint8_t[]> buffer(new uint8_t[len]);


std::vector

wesm · 2017-04-27T02:27:54Z

src/parquet/util/rle-test.cc

-    const int len = RleEncoder::MinBufferSize(bit_width);
-    uint8_t buffer[len];
+    int len = RleEncoder::MinBufferSize(bit_width);
+    std::unique_ptr<uint8_t[]> buffer(new uint8_t[len]);


std::vector

wesm · 2017-04-27T02:28:24Z

tools/parquet-scan.cc

@@ -71,7 +71,7 @@ int main(int argc, char** argv) {
      }
    }

-    int64_t total_rows[num_columns];
+    std::unique_ptr<int64_t[]> total_rows(new int64_t[num_columns]);


std::vector

xhochy · 2017-04-27T16:30:42Z

src/parquet/util/compiler-util.h

@@ -31,8 +31,13 @@
 #undef UNLIKELY
 #endif

+#ifdef _MSC_VER
+#define LIKELY(expr) 0


This should just "return" expr again, otherwise the statement would always be evaluated as false.

You are right! thanks

wesm

+1. I suppose the unit tests don't pass yet?

maxhora · 2017-05-02T13:33:56Z

@wesm , yes, some unit tests are failing on Windows

maxhora force-pushed the PARQUET-679 branch from 81e634e to 3d7955a Compare April 26, 2017 23:16

majetideepak reviewed Apr 27, 2017

View reviewed changes

wesm reviewed Apr 27, 2017

View reviewed changes

xhochy suggested changes Apr 27, 2017

View reviewed changes

maxhora force-pushed the PARQUET-679 branch 5 times, most recently from 90b81d3 to a606a7d Compare April 29, 2017 16:34

PARQUET-679: Local Windows build and Appveyor support

c5c4a8c

maxhora force-pushed the PARQUET-679 branch from a606a7d to c5c4a8c Compare April 30, 2017 19:48

wesm approved these changes May 2, 2017

View reviewed changes

asfgit closed this in e414012 May 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-679: Local Windows build and Appveyor support #313

PARQUET-679: Local Windows build and Appveyor support #313

maxhora commented Apr 26, 2017

majetideepak Apr 26, 2017

wesm commented Apr 27, 2017

wesm left a comment

wesm Apr 27, 2017

wesm Apr 27, 2017

wesm Apr 27, 2017

wesm Apr 27, 2017

wesm Apr 27, 2017

maxhora Apr 28, 2017

wesm Apr 28, 2017

maxhora Apr 28, 2017

wesm Apr 27, 2017

wesm Apr 27, 2017

wesm Apr 27, 2017

wesm Apr 27, 2017

wesm Apr 27, 2017

xhochy Apr 27, 2017

maxhora Apr 28, 2017

wesm left a comment

maxhora commented May 2, 2017 •

edited

Loading


		## Fast setup of building requirements with conda and conda-forge

		To not waste a lot of time to setup the thirdparty build dependecies for

PARQUET-679: Local Windows build and Appveyor support #313

PARQUET-679: Local Windows build and Appveyor support #313

Conversation

maxhora commented Apr 26, 2017

Choose a reason for hiding this comment

wesm commented Apr 27, 2017

wesm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesm left a comment

Choose a reason for hiding this comment

maxhora commented May 2, 2017 • edited Loading

maxhora commented May 2, 2017 •

edited

Loading