imay commented on a change in pull request #3439:
URL: https://github.com/apache/incubator-doris/pull/3439#discussion_r420615579
##########
File path: fe/src/main/java/org/apache/doris/analysis/FunctionCallExpr.java
##########
@@ -605,6 +605,19 @@ public void analyzeImpl(Analyzer analyzer) throws
AnalysisException {
return;
}
+ if
(fnName.getFunction().equalsIgnoreCase("append_trailing_char_if_absent")) {
Review comment:
I think this check is unnecessary, can be removed.
If you keep it, this will make this function more complicate.
##########
File path: be/src/exprs/string_functions.cpp
##########
@@ -212,6 +212,28 @@ StringVal StringFunctions::rpad(
}
return result;
}
+
+StringVal
StringFunctions::append_trailing_char_if_absent(doris_udf::FunctionContext*
context,
+ const doris_udf::StringVal& str, const doris_udf::StringVal&
trailing_char) {
+ if (str.is_null || trailing_char.is_null) {
+ return StringVal::null();
+ }
+ if (str.len == 0) {
+ return trailing_char;
+ }
+ if (str.ptr[str.len-1] == trailing_char.ptr[0]) {
+ return str;
+ }
+
+ StringVal result(context, str.len + trailing_char.len);
+ if (UNLIKELY(result.is_null)) {
Review comment:
I agree with kangkaisen, this check can be removed
##########
File path: be/src/exprs/string_functions.cpp
##########
@@ -212,6 +212,28 @@ StringVal StringFunctions::rpad(
}
return result;
}
+
+StringVal
StringFunctions::append_trailing_char_if_absent(doris_udf::FunctionContext*
context,
+ const doris_udf::StringVal& str, const doris_udf::StringVal&
trailing_char) {
+ if (str.is_null || trailing_char.is_null || trailing_char.len != 1) {
+ return StringVal::null();
+ }
+ if (str.len == 0) {
+ return trailing_char;
+ }
+ if (str.ptr[str.len-1] == trailing_char.ptr[0]) {
Review comment:
```suggestion
if (str.ptr[str.len - 1] == trailing_char.ptr[0]) {
```
##########
File path: be/test/exprs/string_functions_test.cpp
##########
@@ -252,6 +264,26 @@ TEST_F(StringFunctionsTest, null_or_empty) {
delete context;
}
+TEST_F(StringFunctionsTest, append_trailing_char_if_absent) {
+ ASSERT_EQ(StringVal("ac"),
StringFunctions::append_trailing_char_if_absent(ctx,
+ StringVal("a"), StringVal("c")));
+
+ ASSERT_EQ(StringVal("c"),
StringFunctions::append_trailing_char_if_absent(ctx,
+ StringVal("c"), StringVal("c")));
+
+ ASSERT_EQ(StringVal("123c"),
StringFunctions::append_trailing_char_if_absent(ctx,
+ StringVal("123c"), StringVal("c")));
+
+ ASSERT_EQ(StringVal("c"),
StringFunctions::append_trailing_char_if_absent(ctx,
+ StringVal(""), StringVal("c")));
+
+ ASSERT_EQ(StringVal::null(),
StringFunctions::append_trailing_char_if_absent(ctx,
+ StringVal::null(), StringVal("c")));
+
+ ASSERT_EQ(StringVal::null(),
StringFunctions::append_trailing_char_if_absent(ctx,
+ StringVal("a"), StringVal::null()));
Review comment:
Should add test that second argument has multiple characters.
##########
File path: be/src/exprs/string_functions.cpp
##########
@@ -212,6 +212,28 @@ StringVal StringFunctions::rpad(
}
return result;
}
+
+StringVal
StringFunctions::append_trailing_char_if_absent(doris_udf::FunctionContext*
context,
+ const doris_udf::StringVal& str, const doris_udf::StringVal&
trailing_char) {
+ if (str.is_null || trailing_char.is_null || trailing_char.len != 1) {
+ return StringVal::null();
+ }
+ if (str.len == 0) {
+ return trailing_char;
+ }
+ if (str.ptr[str.len-1] == trailing_char.ptr[0]) {
+ return str;
+ }
+
+ StringVal result(context, str.len + trailing_char.len);
Review comment:
```suggestion
StringVal result(context, str.len + 1);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
