You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2022/01/17 15:57:27 UTC
[GitHub] [drill] jnturton commented on a change in pull request #2416: DRILL-8094: Support reverse truncation for split_part udf
jnturton commented on a change in pull request #2416:
URL: https://github.com/apache/drill/pull/2416#discussion_r776967381
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java
##########
@@ -416,16 +417,25 @@ public void setup() {
@Override
public void eval() {
- if (index.value < 1) {
+ if (index.value == 0) {
throw org.apache.drill.common.exceptions.UserException.functionError()
- .message("Index in split_part must be positive, value provided was "
- + index.value).build();
+ .message("Index in split_part can not be zero").build();
}
String inputString = org.apache.drill.exec.expr.fn.impl.
StringFunctionHelpers.getStringFromVarCharHolder(in);
- int arrayIndex = index.value - 1;
- String result =
- (String) com.google.common.collect.Iterables.get(splitter.split(inputString), arrayIndex, "");
+ String result = "";
+ if (index.value < 0) {
+ java.util.List<String> splits = splitter.splitToList(inputString);
+ int size = splits.size();
+ int arrayIndex = size + index.value;
+ if (arrayIndex >= 0) {
Review comment:
For performance we try to avoid branching inside function `eval` methods whenever possible. Can any of these `if` statements be removed? E.g. by using modular arithmetic to calculate the wrapped array index.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java
##########
@@ -416,16 +417,25 @@ public void setup() {
@Override
public void eval() {
- if (index.value < 1) {
+ if (index.value == 0) {
throw org.apache.drill.common.exceptions.UserException.functionError()
- .message("Index in split_part must be positive, value provided was "
- + index.value).build();
+ .message("Index in split_part can not be zero").build();
}
String inputString = org.apache.drill.exec.expr.fn.impl.
StringFunctionHelpers.getStringFromVarCharHolder(in);
- int arrayIndex = index.value - 1;
- String result =
- (String) com.google.common.collect.Iterables.get(splitter.split(inputString), arrayIndex, "");
+ String result = "";
+ if (index.value < 0) {
+ java.util.List<String> splits = splitter.splitToList(inputString);
+ int size = splits.size();
+ int arrayIndex = size + index.value;
+ if (arrayIndex >= 0) {
Review comment:
Okay that does actually appear to be consistent with what happens when arrayIndex is past the end of the array.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/impl/StringFunctions.java
##########
@@ -416,16 +417,25 @@ public void setup() {
@Override
public void eval() {
- if (index.value < 1) {
+ if (index.value == 0) {
throw org.apache.drill.common.exceptions.UserException.functionError()
- .message("Index in split_part must be positive, value provided was "
- + index.value).build();
+ .message("Index in split_part can not be zero").build();
}
String inputString = org.apache.drill.exec.expr.fn.impl.
StringFunctionHelpers.getStringFromVarCharHolder(in);
- int arrayIndex = index.value - 1;
- String result =
- (String) com.google.common.collect.Iterables.get(splitter.split(inputString), arrayIndex, "");
+ String result = "";
+ if (index.value < 0) {
+ java.util.List<String> splits = splitter.splitToList(inputString);
+ int size = splits.size();
+ int arrayIndex = size + index.value;
+ if (arrayIndex >= 0) {
Review comment:
Perhaps in the context of this sort of string processing, branching penalities are not very significant 🤔
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org