From c32ee092fae2cd533f88a1160acff198abc2676d Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Jul 2017 12:10:12 -0400 Subject: [PATCH 1/6] Remove print statement. Disable MSVC 4190 warning Change-Id: I2df30547e45b20609c9712f12ed31488f0397b98 --- python/CMakeLists.txt | 3 +++ python/pyarrow/scalar.pxi | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt index 846e4dd5f6e..74cfd908c48 100644 --- a/python/CMakeLists.txt +++ b/python/CMakeLists.txt @@ -82,6 +82,9 @@ if (NOT MSVC) # Suppress Cython warnings set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused-variable") +else() + # MSVC version of -Wno-return-type-c-linkage + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4190") endif() if ("${COMPILER_FAMILY}" STREQUAL "clang") diff --git a/python/pyarrow/scalar.pxi b/python/pyarrow/scalar.pxi index dec5341ca4a..1f72070cb7e 100644 --- a/python/pyarrow/scalar.pxi +++ b/python/pyarrow/scalar.pxi @@ -169,7 +169,6 @@ cdef class Time64Value(ArrayValue): CTime64Type* dtype = ap.type().get() cdef int64_t val = ap.Value(self.index) - print(val) if dtype.unit() == TimeUnit_MICRO: return (datetime.datetime(1970, 1, 1) + datetime.timedelta(microseconds=val)).time() From 5740f0014f54690f6aaf24f07e1aff8ad14b0c16 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Jul 2017 16:03:10 -0400 Subject: [PATCH 2/6] Add PYARROW_CXXFLAGS option, fix MSVC compiler warnings --- ci/msvc-build.bat | 3 +++ ci/travis_script_python.sh | 1 + python/CMakeLists.txt | 7 +++++++ python/doc/source/development.rst | 1 - python/pyarrow/_parquet.pyx | 2 +- python/pyarrow/includes/libarrow.pxd | 2 +- python/pyarrow/io.pxi | 2 +- python/pyarrow/table.pxi | 10 +++++----- python/pyarrow/types.pxi | 8 ++++---- python/setup.py | 5 +++++ 10 files changed, 28 insertions(+), 13 deletions(-) diff --git a/ci/msvc-build.bat b/ci/msvc-build.bat index 2a537769f82..29cfdd2d9b9 100644 --- a/ci/msvc-build.bat +++ b/ci/msvc-build.bat @@ -114,6 +114,9 @@ popd @rem see PARQUET-1018 pushd python + +set PYARROW_CXXFLAGS="/WX" python setup.py build_ext --inplace --with-parquet --bundle-arrow-cpp bdist_wheel || exit /B py.test pyarrow -v -s --parquet || exit /B + popd diff --git a/ci/travis_script_python.sh b/ci/travis_script_python.sh index 907bc60cd71..9135aaf38e4 100755 --- a/ci/travis_script_python.sh +++ b/ci/travis_script_python.sh @@ -23,6 +23,7 @@ source $TRAVIS_BUILD_DIR/ci/travis_env_common.sh export ARROW_HOME=$ARROW_CPP_INSTALL export PARQUET_HOME=$TRAVIS_BUILD_DIR/parquet-env export LD_LIBRARY_PATH=$ARROW_HOME/lib:$PARQUET_HOME/lib:$LD_LIBRARY_PATH +export PYARROW_CXXFLAGS="-Werror" build_parquet_cpp() { export PARQUET_ARROW_VERSION=$(git rev-parse HEAD) diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt index 74cfd908c48..829cf4b7c0d 100644 --- a/python/CMakeLists.txt +++ b/python/CMakeLists.txt @@ -57,6 +57,8 @@ if("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}") option(PYARROW_BUNDLE_ARROW_CPP "Bundle the Arrow C++ libraries" OFF) + set(PYARROW_CXXFLAGS "" CACHE STRING + "Compiler flags to append when compiling Arrow") endif() find_program(CCACHE_FOUND ccache) @@ -75,6 +77,7 @@ include(CompilerInfo) # Add common flags set(CMAKE_CXX_FLAGS "${CXX_COMMON_FLAGS} ${CMAKE_CXX_FLAGS}") +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${PYARROW_CXXFLAGS}") if (NOT MSVC) # Enable perf and other tools to work properly @@ -85,6 +88,10 @@ if (NOT MSVC) else() # MSVC version of -Wno-return-type-c-linkage set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4190") + + # Cython generates some bitshift expressions that MSVC does not like in + # __Pyx_PyFloat_DivideObjC + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd4293") endif() if ("${COMPILER_FAMILY}" STREQUAL "clang") diff --git a/python/doc/source/development.rst b/python/doc/source/development.rst index 55b3efdad17..d0a1c544dd0 100644 --- a/python/doc/source/development.rst +++ b/python/doc/source/development.rst @@ -267,7 +267,6 @@ Now, we build and install Arrow C++ libraries -DCMAKE_INSTALL_PREFIX=%ARROW_HOME% ^ -DCMAKE_BUILD_TYPE=Release ^ -DARROW_BUILD_TESTS=off ^ - -DARROW_ZLIB_VENDORED=off ^ -DARROW_PYTHON=on .. cmake --build . --target INSTALL --config Release cd ..\.. diff --git a/python/pyarrow/_parquet.pyx b/python/pyarrow/_parquet.pyx index aef661818f4..c940122da5d 100644 --- a/python/pyarrow/_parquet.pyx +++ b/python/pyarrow/_parquet.pyx @@ -636,7 +636,7 @@ cdef class ParquetWriter: elif row_group_size == 0: raise ValueError('Row group size cannot be 0') - cdef int c_row_group_size = row_group_size + cdef int64_t c_row_group_size = row_group_size with nogil: check_status(self.writer.get() diff --git a/python/pyarrow/includes/libarrow.pxd b/python/pyarrow/includes/libarrow.pxd index ffe867b0af0..db6770f586b 100644 --- a/python/pyarrow/includes/libarrow.pxd +++ b/python/pyarrow/includes/libarrow.pxd @@ -369,7 +369,7 @@ cdef extern from "arrow/api.h" namespace "arrow" nogil: shared_ptr[CTable]* table) int num_columns() - int num_rows() + int64_t num_rows() c_bool Equals(const CTable& other) diff --git a/python/pyarrow/io.pxi b/python/pyarrow/io.pxi index 9e4e9078ceb..01c987d286f 100644 --- a/python/pyarrow/io.pxi +++ b/python/pyarrow/io.pxi @@ -923,7 +923,7 @@ cdef class _HdfsClient: cdef c_string c_path = tobytes(path) with nogil: check_status(self.client.get() - .Delete(c_path, recursive)) + .Delete(c_path, recursive == 1)) def open(self, path, mode='rb', buffer_size=None, replication=None, default_block_size=None): diff --git a/python/pyarrow/table.pxi b/python/pyarrow/table.pxi index c1d5a50d487..6277761b7d6 100644 --- a/python/pyarrow/table.pxi +++ b/python/pyarrow/table.pxi @@ -286,7 +286,7 @@ cdef int _schema_from_arrays( c_string c_name vector[shared_ptr[CField]] fields shared_ptr[CDataType] type_ - int K = len(arrays) + Py_ssize_t K = len(arrays) fields.resize(K) @@ -733,7 +733,7 @@ cdef class Table: vector[shared_ptr[CColumn]] columns shared_ptr[CSchema] schema shared_ptr[CTable] table - size_t K = len(arrays) + int i, K = len(arrays) _schema_from_arrays(arrays, names, metadata, &schema) @@ -841,7 +841,7 @@ cdef class Table: self._check_nullptr() return pyarrow_wrap_schema(self.table.schema()) - def column(self, int64_t i): + def column(self, int i): """ Select a column by its numeric index. @@ -855,8 +855,8 @@ cdef class Table: """ cdef: Column column = Column() - int64_t num_columns = self.num_columns - int64_t index + int num_columns = self.num_columns + int index self._check_nullptr() if not -num_columns <= i < num_columns: diff --git a/python/pyarrow/types.pxi b/python/pyarrow/types.pxi index a8d7aa0ee81..fefde55bc2f 100644 --- a/python/pyarrow/types.pxi +++ b/python/pyarrow/types.pxi @@ -281,12 +281,12 @@ cdef class Schema: def __len__(self): return self.schema.num_fields() - def __getitem__(self, int64_t i): + def __getitem__(self, int i): cdef: Field result = Field() - int64_t num_fields = self.schema.num_fields() - int64_t index + int num_fields = self.schema.num_fields() + int index if not -num_fields <= i < num_fields: raise IndexError( @@ -456,7 +456,7 @@ def field(name, DataType type, bint nullable=True, dict metadata=None): convert_metadata(metadata, &c_meta) result.sp_field.reset(new CField(tobytes(name), type.sp_type, - nullable, c_meta)) + nullable == 1, c_meta)) result.field = result.sp_field.get() result.type = type return result diff --git a/python/setup.py b/python/setup.py index 801cd17f440..cc2b3cfc184 100644 --- a/python/setup.py +++ b/python/setup.py @@ -92,6 +92,8 @@ def initialize_options(self): self.extra_cmake_args = os.environ.get('PYARROW_CMAKE_OPTIONS', '') self.build_type = os.environ.get('PYARROW_BUILD_TYPE', 'debug').lower() + self.cmake_cxxflags = os.environ.get('PYARROW_CXXFLAGS', '') + if sys.platform == 'win32': # Cannot do debug builds in Windows unless Python itself is a debug # build @@ -146,6 +148,9 @@ def _run_cmake(self): if self.with_plasma: cmake_options.append('-DPYARROW_BUILD_PLASMA=on') + cmake_options.append('-DPYARROW_CXXFLAGS="{0}"' + .format(self.cmake_cxxflags)) + if self.bundle_arrow_cpp: cmake_options.append('-DPYARROW_BUNDLE_ARROW_CPP=ON') # ARROW-1090: work around CMake rough edges From 2e8f1053d953d40cb84e9f27fbb45887b4c89b0e Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Jul 2017 16:05:04 -0400 Subject: [PATCH 3/6] typo --- ci/msvc-build.bat | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/msvc-build.bat b/ci/msvc-build.bat index 29cfdd2d9b9..04fe2ab62cb 100644 --- a/ci/msvc-build.bat +++ b/ci/msvc-build.bat @@ -115,7 +115,7 @@ popd pushd python -set PYARROW_CXXFLAGS="/WX" +set PYARROW_CXXFLAGS=/WX python setup.py build_ext --inplace --with-parquet --bundle-arrow-cpp bdist_wheel || exit /B py.test pyarrow -v -s --parquet || exit /B From b5a6d9af4dab6dcb6d9fb37d7d8dd87cd62b3d35 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Jul 2017 23:12:46 -0400 Subject: [PATCH 4/6] Supress another clang warning Change-Id: I632372085c35b10dc54c45feda8746462780b029 --- python/CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt index 829cf4b7c0d..460a5435630 100644 --- a/python/CMakeLists.txt +++ b/python/CMakeLists.txt @@ -105,6 +105,7 @@ if ("${COMPILER_FAMILY}" STREQUAL "clang") # Cython warnings in clang set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-parentheses-equality") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-constant-logical-operand") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-missing_declarations") # We have public Cython APIs which return C++ types, which are in an extern # "C" blog (no symbol mangling) and clang doesn't like this From dedcbb9a7985ee10daa7f701a7593ab608186313 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 29 Jul 2017 23:40:06 -0400 Subject: [PATCH 5/6] Fix typo Change-Id: If1f6a1948536f4774756952d2cd4a98cd0a39d2c --- python/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt index 460a5435630..bfae157ed6b 100644 --- a/python/CMakeLists.txt +++ b/python/CMakeLists.txt @@ -105,7 +105,7 @@ if ("${COMPILER_FAMILY}" STREQUAL "clang") # Cython warnings in clang set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-parentheses-equality") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-constant-logical-operand") - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-missing_declarations") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-missing-declarations") # We have public Cython APIs which return C++ types, which are in an extern # "C" blog (no symbol mangling) and clang doesn't like this From 9534ae9d8c2b05902ed5cbe409d721da1c6e8e53 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 30 Jul 2017 10:19:40 -0400 Subject: [PATCH 6/6] Only pass PYARROW_CXXFLAGS when set Change-Id: I94fe0338d2e534355edc67bceafc9cf7e6ce251d --- python/setup.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/setup.py b/python/setup.py index cc2b3cfc184..ebf28cc64e9 100644 --- a/python/setup.py +++ b/python/setup.py @@ -148,8 +148,9 @@ def _run_cmake(self): if self.with_plasma: cmake_options.append('-DPYARROW_BUILD_PLASMA=on') - cmake_options.append('-DPYARROW_CXXFLAGS="{0}"' - .format(self.cmake_cxxflags)) + if len(self.cmake_cxxflags) > 0: + cmake_options.append('-DPYARROW_CXXFLAGS="{0}"' + .format(self.cmake_cxxflags)) if self.bundle_arrow_cpp: cmake_options.append('-DPYARROW_BUNDLE_ARROW_CPP=ON')