[Python][C++] Fix deprecations when building pyarrow

[raulcd]

Describe the bug, including details regarding any error messages, version, and platform.

I've noticed some deprecations when building pyarrow:

  [28/53] Building CXX object CMakeFiles/arrow_python.dir/pyarrow/src/arrow/python/deserialize.cc.o
  In file included from /root/dist/include/arrow/util/cancel.h:25,
                   from /root/dist/include/arrow/io/interfaces.h:28,
                   from /root/dist/include/arrow/io/caching.h:26,
                   from /root/dist/include/arrow/ipc/options.h:24,
                   from /arrow/python/pyarrow/src/arrow/python/serialize.h:23,
                   from /arrow/python/pyarrow/src/arrow/python/deserialize.h:24,
                   from /arrow/python/pyarrow/src/arrow/python/deserialize.cc:18:
  /arrow/python/pyarrow/src/arrow/python/deserialize.cc: In function ‘arrow::Status arrow::py::NdarrayFromBuffer(std::shared_ptr<arrow::Buffer>, std::shared_ptr<arrow::Tensor>*)’:
  /arrow/python/pyarrow/src/arrow/python/deserialize.cc:498:37: warning: ‘arrow::Status arrow::py::ReadSerializedObject(arrow::io::RandomAccessFile*, SerializedPyObject*)’ is deprecated: Deprecated in 18.0.0. Will be removed in 20.0.0 [-Wdeprecated-declarations]
    498 |   RETURN_NOT_OK(ReadSerializedObject(&reader, &object));
        |                 ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
  /root/dist/include/arrow/status.h:57:62: note: in definition of macro ‘ARROW_RETURN_NOT_OK’
     57 |     ::arrow::Status __s = ::arrow::internal::GenericToStatus(status); \
        |                                                              ^~~~~~
  /arrow/python/pyarrow/src/arrow/python/deserialize.cc:498:3: note: in expansion of macro ‘RETURN_NOT_OK’
    498 |   RETURN_NOT_OK(ReadSerializedObject(&reader, &object));
        |   ^~~~~~~~~~~~~
  /arrow/python/pyarrow/src/arrow/python/deserialize.cc:324:8: note: declared here
    324 | Status ReadSerializedObject(io::RandomAccessFile* src, SerializedPyObject* out) {
        |        ^~~~~~~~~~~~~~~~~~~~
  /arrow/python/pyarrow/src/arrow/python/deserialize.cc:499:28: warning: ‘arrow::Status arrow::py::DeserializeNdarray(const SerializedPyObject&, std::shared_ptr<arrow::Tensor>*)’ is deprecated: Deprecated in 18.0.0. Will be removed in 20.0.0 [-Wdeprecated-declarations]
    499 |   return DeserializeNdarray(object, out);
        |          ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
  /arrow/python/pyarrow/src/arrow/python/deserialize.cc:486:8: note: declared here
    486 | Status DeserializeNdarray(const SerializedPyObject& object,
        |        ^~~~~~~~~~~~~~~~~~
  [29/53] Building CXX object CMakeFiles/arrow_python.dir/pyarrow/src/arrow/python/filesystem.cc.o
  [30/53] Building CXX object CMakeFiles/arrow_python.dir/pyarrow/src/arrow/python/python_test.cc.o
  /arrow/python/pyarrow/src/arrow/python/python_test.cc: In function ‘arrow::Status arrow::py::testing::{anonymous}::TestDecimal128OverflowFails()’:
  /arrow/python/pyarrow/src/arrow/python/python_test.cc:666:31: warning: ‘std::shared_ptr<arrow::DataType> arrow::decimal(int32_t, int32_t)’ is deprecated: Deprecated in 18.0. Use `smallest_decimal` instead [-Wdeprecated-declarations]
    666 |   auto type = ::arrow::decimal(38, 38);
        |               ~~~~~~~~~~~~~~~~^~~~~~~~
  In file included from /root/dist/include/arrow/array/statistics.h:25,
                   from /root/dist/include/arrow/array/data.h:27,
                   from /root/dist/include/arrow/array/array_base.h:26,
                   from /root/dist/include/arrow/array.h:41,
                   from /arrow/python/pyarrow/src/arrow/python/python_test.cc:25:
  /root/dist/include/arrow/type_fwd.h:537:27: note: declared here
    537 | std::shared_ptr<DataType> decimal(int32_t precision, int32_t scale);
        |                           ^~~~~~~
  /arrow/python/pyarrow/src/arrow/python/python_test.cc: In function ‘arrow::Status arrow::py::testing::{anonymous}::TestDecimal256OverflowFails()’:
  /arrow/python/pyarrow/src/arrow/python/python_test.cc:692:31: warning: ‘std::shared_ptr<arrow::DataType> arrow::decimal(int32_t, int32_t)’ is deprecated: Deprecated in 18.0. Use `smallest_decimal` instead [-Wdeprecated-declarations]
    692 |   auto type = ::arrow::decimal(76, 76);
        |               ~~~~~~~~~~~~~~~~^~~~~~~~
  /root/dist/include/arrow/type_fwd.h:537:27: note: declared here
    537 | std::shared_ptr<DataType> decimal(int32_t precision, int32_t scale);
        |                           ^~~~~~~

We should fix those.

Component(s)

C++, Python

[raulcd]

In other implementations the build fails if those deprecations are raised (like in C GLib) so those are fixed when the feature is deprecated. We could investigate to do the same for pyarrow and fail the build so pyarrow is also updated when these changes happen.

[kou]

We can use -Werror for it.

FYI: This is a related code in C++:

arrow/cpp/cmake_modules/SetupCxxFlags.cmake

Lines 255 to 389 in f5c340b

	# BUILD_WARNING_LEVEL add warning/error compiler flags. The possible values are
	# - PRODUCTION: Build with `-Wall` but do not add `-Werror`, so warnings do not
	# halt the build.
	# - CHECKIN: Build with `-Wall` and `-Wextra`. Also, add `-Werror` in debug mode
	# so that any important warnings fail the build.
	# - EVERYTHING: Like `CHECKIN`, but possible extra flags depending on the
	# compiler, including `-Wextra`, `-Weverything`, `-pedantic`.
	# This is the most aggressive warning level.
	# Defaults BUILD_WARNING_LEVEL to `CHECKIN`, unless CMAKE_BUILD_TYPE is
	# `RELEASE`, then it will default to `PRODUCTION`. The goal of defaulting to
	# `CHECKIN` is to avoid friction with long response time from CI.
	if(NOT BUILD_WARNING_LEVEL)
	if("${CMAKE_BUILD_TYPE}" STREQUAL "RELEASE")
	set(BUILD_WARNING_LEVEL PRODUCTION)
	else()
	set(BUILD_WARNING_LEVEL CHECKIN)
	endif()
	endif(NOT BUILD_WARNING_LEVEL)
	string(TOUPPER ${BUILD_WARNING_LEVEL} BUILD_WARNING_LEVEL)
	message(STATUS "Arrow build warning level: ${BUILD_WARNING_LEVEL}")
	macro(arrow_add_werror_if_debug)
	# Treat all compiler warnings as errors
	if(MSVC)
	string(APPEND CMAKE_C_FLAGS_DEBUG " /WX")
	string(APPEND CMAKE_CXX_FLAGS_DEBUG " /WX")
	else()
	string(APPEND CMAKE_C_FLAGS_DEBUG " -Werror")
	string(APPEND CMAKE_CXX_FLAGS_DEBUG " -Werror")
	endif()
	endmacro()
	if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
	# Pre-checkin builds
	if(MSVC)
	# https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warnings-by-compiler-version
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /W3")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /wd4365")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /wd4267")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /wd4838")
	elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID STREQUAL
	"Clang")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wextra")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdocumentation")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -DARROW_WARN_DOCUMENTATION")
	if(CMAKE_SYSTEM_NAME STREQUAL "Emscripten")
	# size_t is 32 bit in Emscripten wasm32 - ignore conversion errors
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-shorten-64-to-32")
	else()
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wshorten-64-to-32")
	endif()
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-missing-braces")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-unused-parameter")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-constant-logical-operand")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-return-stack-address")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdate-time")
	elseif(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-conversion")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-sign-conversion")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wdate-time")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wimplicit-fallthrough")
	string(APPEND CXX_ONLY_FLAGS " -Wredundant-move")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wunused-result")
	elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Intel" OR CMAKE_CXX_COMPILER_ID STREQUAL
	"IntelLLVM")
	if(WIN32)
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Wall")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Wno-deprecated")
	else()
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-deprecated")
	endif()
	else()
	message(FATAL_ERROR "${UNKNOWN_COMPILER_MESSAGE}")
	endif()
	arrow_add_werror_if_debug()
	elseif("${BUILD_WARNING_LEVEL}" STREQUAL "EVERYTHING")
	# Pedantic builds for fixing warnings
	if(MSVC)
	string(REPLACE "/W3" "" CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS}")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Wall")
	# https://docs.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level
	# /wdnnnn disables a warning where "nnnn" is a warning number
	elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID STREQUAL
	"Clang")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Weverything")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-c++98-compat")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-c++98-compat-pedantic")
	elseif(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wpedantic")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wextra")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-unused-parameter")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wunused-result")
	elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Intel" OR CMAKE_CXX_COMPILER_ID STREQUAL
	"IntelLLVM")
	if(WIN32)
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Wall")
	else()
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
	endif()
	else()
	message(FATAL_ERROR "${UNKNOWN_COMPILER_MESSAGE}")
	endif()
	arrow_add_werror_if_debug()
	else()
	# Production builds (warning are not treated as errors)
	if(MSVC)
	# https://docs.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level
	# TODO: Enable /Wall and disable individual warnings until build compiles without errors
	# /wdnnnn disables a warning where "nnnn" is a warning number
	string(REPLACE "/W3" "" CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS}")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /W3")
	elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"
	OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang"
	OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
	elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Intel" OR CMAKE_CXX_COMPILER_ID STREQUAL
	"IntelLLVM")
	if(WIN32)
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Wall")
	else()
	set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall")
	endif()
	else()
	message(FATAL_ERROR "${UNKNOWN_COMPILER_MESSAGE}")
	endif()
	endif()

[jorisvandenbossche]

While those warnings are a bit annoying, I am not sure we can do anything about them? (unless there might be a way to hide them specifically)

Those warnings come from functionality we have deprecated ourselves, but as long as we are in the deprecation process, we still include the deprecated functionality, and therefore can trigger some build warnings.

At least that is the case for the serialization one AFAIK. For decimal it looks like an actual usage of a deprecated Arrow C++ function in the PyArrow C++ code, so that can of course be fixed to use a newer variant.

[raulcd]

Thanks @jorisvandenbossche, I see some of the warninds are for the serialize/deserialize functionality in pyarrow which I am not sure how could we mark as not to raise an error vs the others.
I've pushed a PR with changes around a couple places related to Arrow C++ which are the ones that I would expect to get caught.
There are other deprecation warnings around PARQUET_V2 which I am also unsure on whether we should fix those or not:

warning: 'parquet::ParquetVersion::PARQUET_2_0' is deprecated: use PARQUET_2_4 or PARQUET_2_6 for fine-grained feature selection [-Wdeprecated-declarations]

I tested setting PYARROW_CXXFLAGS="-Werror=deprecated-declarations" locally and that would fail the build with the existing deprecations.

[jorisvandenbossche]

There are other deprecation warnings around PARQUET_V2 which I am also unsure on whether we should fix those or not:

Yes, that's another case where we cannot do away with the build warning because we still expose the deprecated C++ feature in python as well.
Now, it has been several years that this is deprecated, so maybe it is time to remove PARQUET_V2 both on the C++ and Python side.