kou commented on code in PR #50112: URL: https://github.com/apache/arrow/pull/50112#discussion_r3383997174
##########
cpp/src/gandiva/precompiled/extended_math_ops_test.cc:
##########
@@ -65,7 +65,8 @@ TEST(TestExtendedMathOps, TestFactorial) {
context.Reset();
factorial_int32(ctx, -5);
- EXPECT_TRUE(context.get_error().find("Factorial of negative") !=
std::string::npos);
+ EXPECT_TRUE(context.get_error().find("FACTORIAL") != std::string::npos);
+ EXPECT_TRUE(context.get_error().find("non-negative") != std::string::npos);
Review Comment:
ditto.
##########
cpp/src/gandiva/precompiled/string_ops.cc:
##########
@@ -2315,62 +2348,79 @@ const char* binary_string(gdv_int64 context, const
char* text, gdv_int32 text_le
return ret;
}
-#define CAST_INT_BIGINT_VARBINARY(OUT_TYPE, TYPE_NAME)
\
- FORCE_INLINE
\
- OUT_TYPE
\
- cast##TYPE_NAME##_varbinary(gdv_int64 context, const char* in, int32_t
in_len) { \
- if (in_len == 0) {
\
- gdv_fn_context_set_error_msg(context, "Can't cast an empty string.");
\
- return -1;
\
- }
\
- char sign = in[0];
\
-
\
- bool negative = false;
\
- if (sign == '-') {
\
- negative = true;
\
- /* Ignores the sign char in the hexadecimal string */
\
- in++;
\
- in_len--;
\
- }
\
-
\
- if (negative && in_len == 0) {
\
- gdv_fn_context_set_error_msg(context,
\
- "Can't cast hexadecimal with only a minus
sign."); \
- return -1;
\
- }
\
-
\
- OUT_TYPE result = 0;
\
- int digit;
\
-
\
- int read_index = 0;
\
- while (read_index < in_len) {
\
- char c1 = in[read_index];
\
- if (isxdigit(c1)) {
\
- digit = to_binary_from_hex(c1);
\
-
\
- OUT_TYPE next = result * 16 - digit;
\
-
\
- if (next > result) {
\
- gdv_fn_context_set_error_msg(context, "Integer overflow.");
\
- return -1;
\
- }
\
- result = next;
\
- read_index++;
\
- } else {
\
- gdv_fn_context_set_error_msg(context,
\
- "The hexadecimal given has invalid
characters."); \
- return -1;
\
- }
\
- }
\
- if (!negative) {
\
- result *= -1;
\
-
\
- if (result < 0) {
\
- gdv_fn_context_set_error_msg(context, "Integer overflow.");
\
- return -1;
\
- }
\
- }
\
- return result;
\
+#define CAST_INT_BIGINT_VARBINARY(OUT_TYPE, TYPE_NAME)
\
+ FORCE_INLINE
\
+ OUT_TYPE
\
+ cast##TYPE_NAME##_varbinary(gdv_int64 context, const char* in, int32_t
in_len) { \
+ const char* in_original = in;
\
+ int32_t in_len_original = in_len;
\
+ if (in_len == 0) {
\
+ gdv_fn_context_set_error_msg(
\
+ context, "CAST_" #TYPE_NAME "_FROM_HEX: can't cast an empty
string"); \
+ return -1;
\
+ }
\
+ char sign = in[0];
\
+
\
+ bool negative = false;
\
+ if (sign == '-') {
\
+ negative = true;
\
+ /* Ignores the sign char in the hexadecimal string */
\
+ in++;
\
+ in_len--;
\
+ }
\
+
\
+ if (negative && in_len == 0) {
\
+ gdv_fn_context_set_error_msg(
\
+ context, "CAST_" #TYPE_NAME
\
+ "_FROM_HEX: can't cast hexadecimal with only a minus
sign"); \
+ return -1;
\
+ }
\
+
\
+ OUT_TYPE result = 0;
\
+ int digit;
\
+
\
+ int read_index = 0;
\
+ while (read_index < in_len) {
\
+ char c1 = in[read_index];
\
+ if (isxdigit(c1)) {
\
+ digit = to_binary_from_hex(c1);
\
+
\
+ OUT_TYPE next = result * 16 - digit;
\
+
\
+ if (next > result) {
\
+ char err_msg[160];
\
+ snprintf(err_msg, sizeof(err_msg),
\
+ "CAST_" #TYPE_NAME
\
+ "_FROM_HEX: integer overflow while reading hex value
'%.*s'", \
+ in_len_original, in_original);
\
Review Comment:
Is `160` safe here? (What is the max value of `in_len_original` here?)
Should we use `std::string` instead of `char[]`?
##########
cpp/src/gandiva/precompiled/decimal_ops_test.cc:
##########
@@ -455,7 +455,7 @@ TEST_F(TestDecimalSql, DivideByZero) {
DecimalScalar128{"201", 20, 3}, DecimalScalar128{"0", 20,
2},
result_precision, result_scale, &overflow);
EXPECT_TRUE(context.has_error());
- EXPECT_EQ(context.get_error(), "divide by zero error");
+ EXPECT_NE(context.get_error().find("divide by zero error"),
std::string::npos);
Review Comment:
Should we use `EXPECT_THAT(::testing::HasSubstr())` like others?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
