ARROW-1491: [C++] Add casting implementations from strings to numbers or boolean #1387

Licht-T · 2017-12-04T13:49:28Z

xhochy

Code looks good, I would like to see one minor comment why we need the separation for INT8.

xhochy · 2017-12-05T12:51:43Z

cpp/src/arrow/compute/kernels/cast.cc

+    std::function<out_type(const std::string&)> cast_func;
+    if (output->type->id() == Type::INT8 || output->type->id() == Type::UINT8) {
+      cast_func = [](const std::string& s) {
+        return boost::numeric_cast<out_type>(boost::lexical_cast<int>(s));


Can you add a comment why this special case is needed?

wesm · 2017-12-05T19:01:58Z

cpp/src/arrow/compute/kernels/cast.cc


+#include <boost/algorithm/string.hpp>
+#include <boost/lexical_cast.hpp>
+#include <boost/numeric/conversion/cast.hpp>


Is it possible to not rely on Boost for this, e.g. are there some alternatives in the STL or that we can access otherwise? I will review the rest in more detail later

Seems that boost::numeric_cast and boost::lexical_cast are not replaceable by STL.
STL has std::to_string, but it does not support small size ints.
http://en.cppreference.com/w/cpp/string/basic_string/to_string

Wouldn't it be ok in the of small size ints just to upcast them? This should not affect performance as it's a small temporary.

wesm · 2017-12-06T18:18:26Z

cpp/src/arrow/compute/kernels/cast.cc

+        return boost::numeric_cast<out_type>(boost::lexical_cast<int>(s));
+      };
+    } else {
+      cast_func = [](const std::string& s) { return boost::lexical_cast<out_type>(s); };


I think C++11 Lambdas actually incur more overhead than an inlined function. We should instead introduce an auxiliary numeric cast functor that does this switch at compile-time (resulting in an inlined function in the inner loop for all possible types) rather than runtime

wesm · 2017-12-06T18:20:07Z

cpp/src/arrow/compute/kernels/cast.cc

+    if (input_array.null_count() > 0) {
+      std::stringstream ss;
+      ss << "Failed to cast NA into " << output->type->ToString();
+      ctx->SetStatus(Status(StatusCode::SerializationError, ss.str()));


If the input has nulls, then the output should have nulls in the same locations

wesm · 2017-12-06T18:20:29Z

cpp/src/arrow/compute/kernels/cast.cc

+      ss << "Failed to cast NA into " << output->type->ToString();
+      ctx->SetStatus(Status(StatusCode::SerializationError, ss.str()));
+      return;
+    }


If the input has nulls, then the output should have nulls in the same locations (like the other cast functions)

wesm · 2017-12-06T18:22:21Z

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

+TEST_F(TestCast, StringToNumber) {
+  CastOptions options;
+
+  vector<bool> is_valid = {true, true, true, true, true};


Can you modify the unit tests to propagate nulls?

wesm · 2017-12-06T18:22:41Z

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

+  CheckCase<StringType, std::string, FloatType, float>(utf8(), v_float, is_valid,
+                                                       float32(), e_float, options);
+  CheckCase<StringType, std::string, DoubleType, double>(utf8(), v_float, is_valid,
+                                                         float64(), e_double, options);


Can you test with a non-zero offset (e.g. foo->Slice(2))?

@wesm It seems that the sliced pattern is already tested in CheckCase method.
https://github.com/Licht-T/arrow/blob/master/cpp/src/arrow/compute/compute-test.cc#L123

wesm · 2017-12-20T21:41:03Z

@Licht-T I will do a bit of work on this patch tomorrow or Friday for further review

Licht-T · 2017-12-25T00:32:28Z

Thanks @wesm! I was busy but now I am okay. Would you mind if I help?

wesm · 2017-12-25T01:09:55Z

Sure please go ahead

…-number-and-bool

Licht-T · 2018-01-10T13:51:50Z

@wesm Now, all fixed.

wesm · 2018-01-10T22:15:16Z

I will review again when I can

xhochy · 2018-01-14T13:01:35Z

This PR looks good besides the dependency on Boost. Probably we need this to get it working but in the longterm, we should get rid of it again.

cpcloud · 2018-02-01T16:47:16Z

cpp/src/arrow/compute/kernels/cast.cc

+typename std::enable_if<std::is_arithmetic<T>::value && !std::is_same<T, int8_t>::value &&
+                            !std::is_same<T, uint8_t>::value,
+                        T>::type
+castStringToNumeric(const std::string& s) {


Capitalize this function.

cpcloud · 2018-02-01T16:47:22Z

cpp/src/arrow/compute/kernels/cast.cc

+template <typename T>
+typename std::enable_if<std::is_same<T, int8_t>::value || std::is_same<T, uint8_t>::value,
+                        T>::type
+castStringToNumeric(const std::string& s) {


Capitalize.

cpcloud · 2018-02-01T16:49:07Z

cpp/src/arrow/compute/kernels/cast.cc

+
+    auto out_data = GetMutableValues<out_type>(output, 1);
+
+    std::function<out_type(const std::string&)> cast_func;


Is this variable used anywhere? It looks like you might've replaced it with the castStringToNumeric function.

cpcloud · 2018-02-01T16:49:37Z

cpp/src/arrow/compute/kernels/cast.cc

+
+      try {
+        *out_data++ = castStringToNumeric<out_type>(s);
+      } catch (...) {


Is there a specific exception that can be caught here?

I'm concerned about propagating the actual error message instead of just saying "Cast from X to Y failed".

cpcloud · 2018-02-16T20:37:23Z

I'm taking over this PR, will put up a new one based on this one.

The implementation for numbers uses C++ `istringstream`. This makes casting a bit lenient (it will probably accept whitespace). This is a rewrite of #1387 Author: Antoine Pitrou <antoine@python.org> Closes #2362 from pitrou/ARROW-1491-cast-string-to-number and squashes the following commits: c7db1b0 <Antoine Pitrou> Use trait "enable_if_number" 5a9c9a0 <Antoine Pitrou> Use `istringstream` for locale-independent parsing c84aac8 <Antoine Pitrou> ARROW-1491: Add casting from strings to numbers and booleans

wesm · 2018-08-06T23:26:07Z

Superseded by #2362

Licht-T force-pushed the feature-string-to-number-and-bool branch from a13c5e0 to 491e5eb Compare December 4, 2017 14:07

ENH: Add casting implementations from strings to numbers or boolean

d614dd4

Licht-T force-pushed the feature-string-to-number-and-bool branch from 491e5eb to a12eb38 Compare December 4, 2017 14:55

TST: Add tests for casting from strings to numbers or boolean

8a6470c

Licht-T force-pushed the feature-string-to-number-and-bool branch from a12eb38 to 8a6470c Compare December 4, 2017 16:16

Add /bigobj into CMAKE_CXX_FLAGS if the compiler is MSVC

32a9170

xhochy requested changes Dec 5, 2017

View reviewed changes

wesm reviewed Dec 5, 2017

View reviewed changes

wesm reviewed Dec 6, 2017

View reviewed changes

Licht-T added 4 commits December 29, 2017 19:28

Merge remote-tracking branch 'original/master' into feature-string-to…

6252cb0

…-number-and-bool

ENH: Add String to Number/Boolean cast support with NA

5d3871d

TST: Add test of the String to Number/Boolean cast with NA

24dec32

Replace lambda cast-function to template one

fbd5907

cpcloud requested changes Feb 1, 2018

View reviewed changes

Licht-T added 3 commits May 5, 2018 15:45

Merge branch 'master' into feature-string-to-number-and-bool

4d0d781

Capitalize castStringToNumeric method

6020f3d

Remove unused variable

fdbd3e0

pitrou mentioned this pull request Aug 2, 2018

ARROW-1491: [C++] Add casting from strings to numbers and booleans #2362

Closed

wesm closed this Aug 6, 2018

asfimport mentioned this pull request Aug 6, 2018

[C++] Add casting implementations from strings to numbers or boolean #17510

Closed


		auto out_data = GetMutableValues<out_type>(output, 1);

		std::function<out_type(const std::string&)> cast_func;

ARROW-1491: [C++] Add casting implementations from strings to numbers or boolean #1387

ARROW-1491: [C++] Add casting implementations from strings to numbers or boolean #1387

Uh oh!

Conversation

Licht-T commented Dec 4, 2017

Uh oh!

xhochy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Dec 20, 2017

Uh oh!

Licht-T commented Dec 25, 2017

Uh oh!

wesm commented Dec 25, 2017

Uh oh!

Licht-T commented Jan 10, 2018

Uh oh!

wesm commented Jan 10, 2018

Uh oh!

xhochy commented Jan 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpcloud commented Feb 16, 2018

Uh oh!

wesm commented Aug 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants