Jackie-Jiang commented on a change in pull request #7485:
URL: https://github.com/apache/pinot/pull/7485#discussion_r715954242
##########
File path:
pinot-core/src/test/java/org/apache/pinot/core/data/function/InbuiltFunctionEvaluatorTest.java
##########
@@ -120,10 +128,27 @@ public void testStateSharedBetweenRowsForExecution()
assertEquals(evaluator.evaluate(row), "test test test ");
}
+ @Test
+ public void testExceptionDuringInbuiltFunctionEvaluator()
+ throws Exception {
+ String expression = "fromDateTime('2020-01-01T00:00:00Z',
\"invalid_identifier\")";
+ InbuiltFunctionEvaluator evaluator = new
InbuiltFunctionEvaluator(expression);
+ assertEquals(1, evaluator.getArguments().size());
+ GenericRow row = new GenericRow();
+ try {
+ evaluator.evaluate(row);
+ fail();
+ } catch (Exception e) {
+ assertTrue(e instanceof RuntimeException);
Review comment:
Should we check the actual exception message to verify the expected
behavior?
##########
File path:
pinot-core/src/test/java/org/apache/pinot/core/data/function/InbuiltFunctionEvaluatorTest.java
##########
@@ -120,10 +128,27 @@ public void testStateSharedBetweenRowsForExecution()
assertEquals(evaluator.evaluate(row), "test test test ");
}
+ @Test
+ public void testExceptionDuringInbuiltFunctionEvaluator()
+ throws Exception {
+ String expression = "fromDateTime('2020-01-01T00:00:00Z',
\"invalid_identifier\")";
+ InbuiltFunctionEvaluator evaluator = new
InbuiltFunctionEvaluator(expression);
+ assertEquals(1, evaluator.getArguments().size());
Review comment:
First argument is actual, second argument is expected. It will be easier
for debugging when it fails
```suggestion
assertEquals(evaluator.getArguments().size(), 1);
```
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
##########
@@ -112,22 +113,39 @@ public Object evaluate(Object[] values) {
@Override
public Object execute(GenericRow row) {
- int numArguments = _argumentNodes.length;
- for (int i = 0; i < numArguments; i++) {
- _arguments[i] = _argumentNodes[i].execute(row);
+ try {
+ int numArguments = _argumentNodes.length;
+ for (int i = 0; i < numArguments; i++) {
+ _arguments[i] = _argumentNodes[i].execute(row);
+ }
+ _functionInvoker.convertTypes(_arguments);
+ return _functionInvoker.invoke(_arguments);
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Caught exception while execute function: " +
_functionInvoker.getMethod().getName()
+ + " with argument nodes: " + Arrays.toString(_argumentNodes),
e);
}
- _functionInvoker.convertTypes(_arguments);
- return _functionInvoker.invoke(_arguments);
}
@Override
public Object execute(Object[] values) {
- int numArguments = _argumentNodes.length;
- for (int i = 0; i < numArguments; i++) {
- _arguments[i] = _argumentNodes[i].execute(values);
+ try {
+ int numArguments = _argumentNodes.length;
+ for (int i = 0; i < numArguments; i++) {
+ _arguments[i] = _argumentNodes[i].execute(values);
+ }
+ _functionInvoker.convertTypes(_arguments);
+ return _functionInvoker.invoke(_arguments);
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Caught exception while execute function: " +
_functionInvoker.getMethod().getName()
Review comment:
Same
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
##########
@@ -112,22 +113,39 @@ public Object evaluate(Object[] values) {
@Override
public Object execute(GenericRow row) {
- int numArguments = _argumentNodes.length;
- for (int i = 0; i < numArguments; i++) {
- _arguments[i] = _argumentNodes[i].execute(row);
+ try {
+ int numArguments = _argumentNodes.length;
+ for (int i = 0; i < numArguments; i++) {
+ _arguments[i] = _argumentNodes[i].execute(row);
+ }
+ _functionInvoker.convertTypes(_arguments);
+ return _functionInvoker.invoke(_arguments);
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Caught exception while execute function: " +
_functionInvoker.getMethod().getName()
Review comment:
(nit)
```suggestion
"Caught exception while executing function: " +
_functionInvoker.getMethod().getName()
```
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java
##########
@@ -112,22 +113,39 @@ public Object evaluate(Object[] values) {
@Override
public Object execute(GenericRow row) {
- int numArguments = _argumentNodes.length;
- for (int i = 0; i < numArguments; i++) {
- _arguments[i] = _argumentNodes[i].execute(row);
+ try {
+ int numArguments = _argumentNodes.length;
+ for (int i = 0; i < numArguments; i++) {
+ _arguments[i] = _argumentNodes[i].execute(row);
+ }
+ _functionInvoker.convertTypes(_arguments);
+ return _functionInvoker.invoke(_arguments);
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Caught exception while execute function: " +
_functionInvoker.getMethod().getName()
+ + " with argument nodes: " + Arrays.toString(_argumentNodes),
e);
}
- _functionInvoker.convertTypes(_arguments);
- return _functionInvoker.invoke(_arguments);
}
@Override
public Object execute(Object[] values) {
- int numArguments = _argumentNodes.length;
- for (int i = 0; i < numArguments; i++) {
- _arguments[i] = _argumentNodes[i].execute(values);
+ try {
+ int numArguments = _argumentNodes.length;
+ for (int i = 0; i < numArguments; i++) {
+ _arguments[i] = _argumentNodes[i].execute(values);
+ }
+ _functionInvoker.convertTypes(_arguments);
+ return _functionInvoker.invoke(_arguments);
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Caught exception while execute function: " +
_functionInvoker.getMethod().getName()
+ + " with argument nodes: " + Arrays.toString(_argumentNodes),
e);
}
- _functionInvoker.convertTypes(_arguments);
- return _functionInvoker.invoke(_arguments);
+ }
+
+ @Override
+ public String toString() {
+ return String.format("[FunctionExecutionNode[functionName:%s]]",
_functionInvoker.getMethod().getName());
Review comment:
We should also include the `_argumentNodes`. I'd suggest returning the
format of `function(column, 'const')` for readability and easier debugging. You
may refer to the `ExpressionContext` class on how we serialize the expression.
##########
File path:
pinot-core/src/test/java/org/apache/pinot/core/data/function/InbuiltFunctionEvaluatorTest.java
##########
@@ -120,10 +128,27 @@ public void testStateSharedBetweenRowsForExecution()
assertEquals(evaluator.evaluate(row), "test test test ");
}
+ @Test
+ public void testExceptionDuringInbuiltFunctionEvaluator()
+ throws Exception {
+ String expression = "fromDateTime('2020-01-01T00:00:00Z',
\"invalid_identifier\")";
+ InbuiltFunctionEvaluator evaluator = new
InbuiltFunctionEvaluator(expression);
+ assertEquals(1, evaluator.getArguments().size());
+ GenericRow row = new GenericRow();
+ try {
+ evaluator.evaluate(row);
+ fail();
+ } catch (Exception e) {
+ assertTrue(e instanceof RuntimeException);
+ assertTrue(e.getCause() instanceof IllegalStateException);
+ }
+ }
+
private static class MyFunc {
String _baseString = "";
String appendToStringAndReturn(String addedString) {
+ Preconditions.checkNotNull(addedString);
Review comment:
Seems unrelated?
##########
File path:
pinot-core/src/test/java/org/apache/pinot/core/data/function/InbuiltFunctionEvaluatorTest.java
##########
@@ -18,18 +18,28 @@
*/
package org.apache.pinot.core.data.function;
+import com.google.common.base.Preconditions;
import java.util.Collections;
import org.apache.pinot.common.function.FunctionRegistry;
import org.apache.pinot.segment.local.function.InbuiltFunctionEvaluator;
import org.apache.pinot.spi.data.readers.GenericRow;
+import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertTrue;
+import static org.testng.AssertJUnit.fail;
public class InbuiltFunctionEvaluatorTest {
+ @BeforeClass
+ public void setUp()
Review comment:
This function is specific to one test, so I suggest keeping it within
the test
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
