ARROW-2153/ARROW-2160: [C++/Python] Fix decimal precision inference

cpp/CMakeLists.txt

@@ -577,11 +577,13 @@ set(ARROW_LINK_LIBS
 
 set(ARROW_SHARED_PRIVATE_LINK_LIBS
   ${BOOST_SYSTEM_LIBRARY}
-  ${BOOST_FILESYSTEM_LIBRARY})
+  ${BOOST_FILESYSTEM_LIBRARY}
+  ${BOOST_REGEX_LIBRARY})
 
 set(ARROW_STATIC_PRIVATE_LINK_LIBS
   ${BOOST_SYSTEM_LIBRARY}
-  ${BOOST_FILESYSTEM_LIBRARY})
+  ${BOOST_FILESYSTEM_LIBRARY}
+  ${BOOST_REGEX_LIBRARY})
 
 if (NOT MSVC)
   set(ARROW_LINK_LIBS

cpp/cmake_modules/ThirdpartyToolchain.cmake

@@ -157,16 +157,20 @@ if (ARROW_BOOST_VENDORED)
     "${BOOST_LIB_DIR}/${CMAKE_STATIC_LIBRARY_PREFIX}boost_system${CMAKE_STATIC_LIBRARY_SUFFIX}")
   set(BOOST_STATIC_FILESYSTEM_LIBRARY
     "${BOOST_LIB_DIR}/${CMAKE_STATIC_LIBRARY_PREFIX}boost_filesystem${CMAKE_STATIC_LIBRARY_SUFFIX}")
+  set(BOOST_STATIC_REGEX_LIBRARY
+          "${BOOST_LIB_DIR}/${CMAKE_STATIC_LIBRARY_PREFIX}boost_regex${CMAKE_STATIC_LIBRARY_SUFFIX}")
   set(BOOST_SYSTEM_LIBRARY "${BOOST_STATIC_SYSTEM_LIBRARY}")
   set(BOOST_FILESYSTEM_LIBRARY "${BOOST_STATIC_FILESYSTEM_LIBRARY}")
+  set(BOOST_REGEX_LIBRARY "${BOOST_STATIC_REGEX_LIBRARY}")
   if (ARROW_BOOST_HEADER_ONLY)
     set(BOOST_BUILD_PRODUCTS)
     set(BOOST_CONFIGURE_COMMAND "")
     set(BOOST_BUILD_COMMAND "")
   else()
     set(BOOST_BUILD_PRODUCTS
       ${BOOST_SYSTEM_LIBRARY}
-      ${BOOST_FILESYSTEM_LIBRARY})
+      ${BOOST_FILESYSTEM_LIBRARY}
+      ${BOOST_REGEX_LIBRARY})
     set(BOOST_CONFIGURE_COMMAND
       "./bootstrap.sh"
       "--prefix=${BOOST_PREFIX}"
@@ -210,16 +214,19 @@ else()
     if (ARROW_BOOST_HEADER_ONLY)
       find_package(Boost REQUIRED)
     else()
-      find_package(Boost COMPONENTS system filesystem REQUIRED)
+      find_package(Boost COMPONENTS system filesystem regex REQUIRED)
       if ("${CMAKE_BUILD_TYPE}" STREQUAL "DEBUG")
         set(BOOST_SHARED_SYSTEM_LIBRARY ${Boost_SYSTEM_LIBRARY_DEBUG})
         set(BOOST_SHARED_FILESYSTEM_LIBRARY ${Boost_FILESYSTEM_LIBRARY_DEBUG})
+        set(BOOST_SHARED_REGEX_LIBRARY ${Boost_REGEX_LIBRARY_DEBUG})
       else()
         set(BOOST_SHARED_SYSTEM_LIBRARY ${Boost_SYSTEM_LIBRARY_RELEASE})
         set(BOOST_SHARED_FILESYSTEM_LIBRARY ${Boost_FILESYSTEM_LIBRARY_RELEASE})
+        set(BOOST_SHARED_REGEX_LIBRARY ${Boost_REGEX_LIBRARY_RELEASE})
       endif()
       set(BOOST_SYSTEM_LIBRARY boost_system_shared)
       set(BOOST_FILESYSTEM_LIBRARY boost_filesystem_shared)
+      set(BOOST_REGEX_LIBRARY boost_regex_shared)
     endif()
   else()
     # Find static boost headers and libs
@@ -228,16 +235,19 @@ else()
     if (ARROW_BOOST_HEADER_ONLY)
       find_package(Boost REQUIRED)
     else()
-      find_package(Boost COMPONENTS system filesystem REQUIRED)
+      find_package(Boost COMPONENTS system filesystem regex REQUIRED)
       if ("${CMAKE_BUILD_TYPE}" STREQUAL "DEBUG")
         set(BOOST_STATIC_SYSTEM_LIBRARY ${Boost_SYSTEM_LIBRARY_DEBUG})
         set(BOOST_STATIC_FILESYSTEM_LIBRARY ${Boost_FILESYSTEM_LIBRARY_DEBUG})
+        set(BOOST_STATIC_REGEX_LIBRARY ${Boost_REGEX_LIBRARY_DEBUG})
       else()
         set(BOOST_STATIC_SYSTEM_LIBRARY ${Boost_SYSTEM_LIBRARY_RELEASE})
         set(BOOST_STATIC_FILESYSTEM_LIBRARY ${Boost_FILESYSTEM_LIBRARY_RELEASE})
+        set(BOOST_STATIC_REGEX_LIBRARY ${Boost_REGEX_LIBRARY_RELEASE})
       endif()
       set(BOOST_SYSTEM_LIBRARY boost_system_static)
       set(BOOST_FILESYSTEM_LIBRARY boost_filesystem_static)
+      set(BOOST_REGEX_LIBRARY boost_regex_static)
     endif()
   endif()
 endif()
@@ -254,7 +264,11 @@ if (NOT ARROW_BOOST_HEADER_ONLY)
       STATIC_LIB "${BOOST_STATIC_FILESYSTEM_LIBRARY}"
       SHARED_LIB "${BOOST_SHARED_FILESYSTEM_LIBRARY}")
 
-  SET(ARROW_BOOST_LIBS boost_system boost_filesystem)
+  ADD_THIRDPARTY_LIB(boost_regex
+      STATIC_LIB "${BOOST_STATIC_REGEX_LIBRARY}"
+      SHARED_LIB "${BOOST_SHARED_REGEX_LIBRARY}")
+
+  SET(ARROW_BOOST_LIBS boost_system boost_filesystem boost_regex)
 endif()
 
 include_directories(SYSTEM ${Boost_INCLUDE_DIR})

cpp/src/arrow/python/helpers.cc

@@ -99,7 +99,8 @@ Status InferDecimalPrecisionAndScale(PyObject* python_decimal, int32_t* precisio
   DCHECK_NE(precision, NULLPTR);
   DCHECK_NE(scale, NULLPTR);
 
-  OwnedRef as_tuple(PyObject_CallMethod(python_decimal, "as_tuple", "()"));
+  // TODO(phillipc): Make sure we perform PyDecimal_Check(python_decimal) as a DCHECK
+  OwnedRef as_tuple(PyObject_CallMethod(python_decimal, "as_tuple", ""));
   RETURN_IF_PYERROR();
   DCHECK(PyTuple_Check(as_tuple.obj()));
 
@@ -117,7 +118,21 @@ Status InferDecimalPrecisionAndScale(PyObject* python_decimal, int32_t* precisio
   const auto exponent = static_cast<int32_t>(PyLong_AsLong(py_exponent.obj()));
   RETURN_IF_PYERROR();
 
-  *precision = num_digits;
+  const int32_t abs_exponent = std::abs(exponent);
+
+  int32_t num_additional_zeros;
+
+  if (num_digits < abs_exponent) {
+    DCHECK_NE(exponent, 0) << "exponent should never be zero here";
+
+    // we have leading/trailing zeros, leading if exponent is negative
+    num_additional_zeros = exponent < 0 ? abs_exponent - num_digits : exponent;
+  } else {
+    // we can use the number of digits as the precision
+    num_additional_zeros = 0;
+  }
+
+  *precision = num_digits + num_additional_zeros;
   *scale = -exponent;
   return Status::OK();
 }

cpp/src/arrow/python/numpy_to_arrow.cc

@@ -766,10 +766,7 @@ Status NumPyConverter::ConvertDecimals() {
       RETURN_NOT_OK(internal::InferDecimalPrecisionAndScale(objects[i], &tmp_precision,
                                                             &tmp_scale));
       precision = std::max(precision, tmp_precision);
-
-      if (std::abs(desired_scale) < std::abs(tmp_scale)) {
-        desired_scale = tmp_scale;
-      }
+      desired_scale = std::max(desired_scale, tmp_scale);
     }
 
     type_ = ::arrow::decimal(precision, desired_scale);

cpp/src/arrow/python/python-test.cc

@@ -145,6 +145,57 @@ TEST_F(DecimalTest, TestInferPrecisionAndNegativeScale) {
   ASSERT_EQ(expected_scale, scale);
 }
 
+TEST_F(DecimalTest, TestInferAllLeadingZeros) {
+  std::string decimal_string("0.001");
+  OwnedRef python_decimal(this->CreatePythonDecimal(decimal_string));
+
+  int32_t precision;
+  int32_t scale;
+
+  ASSERT_OK(
+      internal::InferDecimalPrecisionAndScale(python_decimal.obj(), &precision, &scale));
+
+  const int32_t expected_precision = 3;
+  const int32_t expected_scale = 3;
+
+  ASSERT_EQ(expected_precision, precision);
+  ASSERT_EQ(expected_scale, scale);
+}
+
+TEST_F(DecimalTest, TestInferAllLeadingZerosExponentialNotationPositive) {
+  std::string decimal_string("0.01E5");
+  OwnedRef python_decimal(this->CreatePythonDecimal(decimal_string));
+
+  int32_t precision;
+  int32_t scale;
+
+  ASSERT_OK(
+      internal::InferDecimalPrecisionAndScale(python_decimal.obj(), &precision, &scale));
+
+  const int32_t expected_precision = 4;
+  const int32_t expected_scale = -3;
+
+  ASSERT_EQ(expected_precision, precision);
+  ASSERT_EQ(expected_scale, scale);
+}
+
+TEST_F(DecimalTest, TestInferAllLeadingZerosExponentialNotationNegative) {
+  std::string decimal_string("0.01E3");
+  OwnedRef python_decimal(this->CreatePythonDecimal(decimal_string));
+
+  int32_t precision;
+  int32_t scale;
+
+  ASSERT_OK(
+      internal::InferDecimalPrecisionAndScale(python_decimal.obj(), &precision, &scale));
+
+  const int32_t expected_precision = 1;
+  const int32_t expected_scale = -1;
+
+  ASSERT_EQ(expected_precision, precision);
+  ASSERT_EQ(expected_scale, scale);
+}
+
 TEST(PandasConversionTest, TestObjectBlockWriteFails) {
   StringBuilder builder;
   const char value[] = {'\xf1', '\0'};
@@ -241,5 +292,43 @@ TEST_F(DecimalTest, TestOverflowFails) {
                                                             decimal_type, &value));
 }
 
+
+TEST_F(DecimalTest, SimpleInference) {
+  OwnedRef value(this->CreatePythonDecimal("0.01"));
+  ASSERT_NE(value.obj(), nullptr);
+  int32_t precision;
+  int32_t scale;
+  ASSERT_OK(internal::InferDecimalPrecisionAndScale(value.obj(), &precision, &scale));
+  ASSERT_EQ(2, precision);
+  ASSERT_EQ(2, scale);
+}
+
+
+TEST_F(DecimalTest, TestMixedPrecisionAndScaleSequenceConvert) {
+
+  PyAcquireGIL lock;
+  MemoryPool* pool = default_memory_pool();
+  std::shared_ptr<Array> arr;
+
+  OwnedRef list_ref(PyList_New(2));
+  PyObject* list = list_ref.obj();
+
+  ASSERT_NE(list, nullptr);
+
+  PyObject* value1 = this->CreatePythonDecimal("0.01").detach();
+  ASSERT_NE(value1, nullptr);
+
+  PyObject* value2 = this->CreatePythonDecimal("0.001").detach();
+  ASSERT_NE(value2, nullptr);
+
+  // This steals a reference to each object, so we don't need to decref them later
+  // just the list
+
+  ASSERT_EQ(PyList_SetItem(list, 0, value1), 0);
+  ASSERT_EQ(PyList_SetItem(list, 1, value2), 0);
+
+  ASSERT_OK(ConvertPySequence(list, pool, &arr));
+}
+
 }  // namespace py
 }  // namespace arrow

cpp/src/arrow/util/decimal-test.cc

@@ -37,15 +37,15 @@ class DecimalTestFixture : public ::testing::Test {
 
 TEST_F(DecimalTestFixture, TestToString) {
   Decimal128 decimal(this->integer_value_);
-  int scale = 5;
+  int32_t scale = 5;
   std::string result = decimal.ToString(scale);
   ASSERT_EQ(result, this->string_value_);
 }
 
 TEST_F(DecimalTestFixture, TestFromString) {
   Decimal128 expected(this->integer_value_);
   Decimal128 result;
-  int precision, scale;
+  int32_t precision, scale;
   ASSERT_OK(Decimal128::FromString(this->string_value_, &result, &precision, &scale));
   ASSERT_EQ(result, expected);
   ASSERT_EQ(precision, 8);
@@ -55,8 +55,8 @@ TEST_F(DecimalTestFixture, TestFromString) {
 TEST_F(DecimalTestFixture, TestStringStartingWithPlus) {
   std::string plus_value("+234.234");
   Decimal128 out;
-  int scale;
-  int precision;
+  int32_t scale;
+  int32_t precision;
   ASSERT_OK(Decimal128::FromString(plus_value, &out, &precision, &scale));
   ASSERT_EQ(234234, out);
   ASSERT_EQ(6, precision);
@@ -67,8 +67,8 @@ TEST_F(DecimalTestFixture, TestStringStartingWithPlus128) {
   std::string plus_value("+2342394230592.232349023094");
   Decimal128 expected_value("2342394230592232349023094");
   Decimal128 out;
-  int scale;
-  int precision;
+  int32_t scale;
+  int32_t precision;
   ASSERT_OK(Decimal128::FromString(plus_value, &out, &precision, &scale));
   ASSERT_EQ(expected_value, out);
   ASSERT_EQ(25, precision);
@@ -90,9 +90,7 @@ TEST(DecimalTest, TestFromDecimalString128) {
   Decimal128 result;
   ASSERT_OK(Decimal128::FromString(string_value, &result));
   Decimal128 expected(static_cast<int64_t>(-230492239423435324));
-  expected *= 100;
-  expected -= 12;
-  ASSERT_EQ(result, expected);
+  ASSERT_EQ(result, expected * 100 - 12);
 
   // Sanity check that our number is actually using more than 64 bits
   ASSERT_NE(result.high_bits(), 0);
@@ -194,36 +192,36 @@ TEST(DecimalTest, TestInvalidInputWithLeadingZeros) {
 TEST(DecimalZerosTest, LeadingZerosNoDecimalPoint) {
   std::string string_value("0000000");
   Decimal128 d;
-  int precision;
-  int scale;
+  int32_t precision;
+  int32_t scale;
   ASSERT_OK(Decimal128::FromString(string_value, &d, &precision, &scale));
-  ASSERT_EQ(precision, 7);
-  ASSERT_EQ(scale, 0);
-  ASSERT_EQ(d, 0);
+  ASSERT_EQ(0, precision);
+  ASSERT_EQ(0, scale);
+  ASSERT_EQ(0, d);
 }
 
 TEST(DecimalZerosTest, LeadingZerosDecimalPoint) {
   std::string string_value("000.0000");
   Decimal128 d;
-  int precision;
-  int scale;
+  int32_t precision;
+  int32_t scale;
   ASSERT_OK(Decimal128::FromString(string_value, &d, &precision, &scale));
   // We explicitly do not support this for now, otherwise this would be ASSERT_EQ
-  ASSERT_NE(precision, 7);
+  ASSERT_EQ(4, precision);
 
-  ASSERT_EQ(scale, 4);
-  ASSERT_EQ(d, 0);
+  ASSERT_EQ(4, scale);
+  ASSERT_EQ(0, d);
 }
 
 TEST(DecimalZerosTest, NoLeadingZerosDecimalPoint) {
   std::string string_value(".00000");
   Decimal128 d;
-  int precision;
-  int scale;
+  int32_t precision;
+  int32_t scale;
   ASSERT_OK(Decimal128::FromString(string_value, &d, &precision, &scale));
-  ASSERT_EQ(precision, 5);
-  ASSERT_EQ(scale, 5);
-  ASSERT_EQ(d, 0);
+  ASSERT_EQ(5, precision);
+  ASSERT_EQ(5, scale);
+  ASSERT_EQ(0, d);
 }
 
 template <typename T>
@@ -375,4 +373,14 @@ TEST(Decimal128Test, TestSmallNumberFormat) {
   ASSERT_EQ(expected, result);
 }
 
+TEST(Decimal128Test, TestNoDecimalPointExponential) {
+  Decimal128 value;
+  int32_t precision;
+  int32_t scale;
+  ASSERT_OK(Decimal128::FromString("1E1", &value, &precision, &scale));
+  ASSERT_EQ(1, value.low_bits());
+  ASSERT_EQ(1, precision);
+  ASSERT_EQ(-1, scale);
+}
+
 }  // namespace arrow