zhztheplayer commented on code in PR #10047:
URL:
https://github.com/apache/incubator-gluten/pull/10047#discussion_r2163563908
##########
gluten-flink/planner/src/main/java/org/apache/gluten/rexnode/functions/BasicCompareOperatorConverters.java:
##########
@@ -59,18 +69,24 @@ public StringNumberCompareRexCallConverter(String
functionName) {
}
@Override
- public boolean isSupported(RexCall callNode, RexConversionContext context) {
+ public ValidationResult doValidate(RexCall callNode, RexConversionContext
context) {
// This converter supports string and numeric comparison functions.
List<Type> paramTypes =
callNode.getOperands().stream()
.map(param -> RexNodeConverter.toType(param.getType()))
.collect(Collectors.toList());
- if ((TypeUtils.isNumericType(paramTypes.get(0)) &&
TypeUtils.isStringType(paramTypes.get(1)))
- || (TypeUtils.isStringType(paramTypes.get(0))
- && TypeUtils.isNumericType(paramTypes.get(1)))) {
- return true;
+ boolean typesValidate =
+ (TypeUtils.isNumericType(paramTypes.get(0)) &&
TypeUtils.isStringType(paramTypes.get(1)))
+ || (TypeUtils.isStringType(paramTypes.get(0))
+ && TypeUtils.isNumericType(paramTypes.get(1)));
+ if (!typesValidate) {
+ String message =
+ String.format(
+ "String and numeric comparison operation requires one string and
one numeric operand, but found: %s",
+ getFunctionProtoTypeName(callNode));
+ return ValidationResult.failure(message);
Review Comment:
Just my two cents, non-blocking:
I feel ideally Velox should do this sort of validation for us so we don't
have to duplicate the work from Gluten's code.
So far the Velox4J utility `Evaluator` will validate the expression during
initialization:
For example, say I am creating a `Evaludator` with incorrect 3 way inputs
for a "multiply" function (it should be 2),
```java
final Evaluation expr =
new Evaluation(
new CallTypedExpr(
new BigIntType(),
List.of(
FieldAccessTypedExpr.create(new BigIntType(), "c0"),
FieldAccessTypedExpr.create(new BigIntType(), "c0"),
FieldAccessTypedExpr.create(new BigIntType(), "a1")),
"multiply"),
Config.empty(),
ConnectorConfig.empty());
final Evaluator evaluator =
session.evaluationOps().createEvaluator(expr);
```
I'll get error
```java
io.github.zhztheplayer.velox4j.exception.VeloxException: Exception:
VeloxUserError
Error Source: USER
Error Code: INVALID_ARGUMENT
Reason: Scalar function multiply not registered with arguments: (BIGINT,
BIGINT, BIGINT). Found function registered with the following signatures:
((decimal(i1,i5),decimal(i2,i6)) -> decimal(i3,i7))
((real,real) -> real)
((double,double) -> double)
((bigint,bigint) -> bigint)
((integer,integer) -> integer)
((smallint,smallint) -> smallint)
((tinyint,tinyint) -> tinyint)
Retriable: False
Function: compileRewrittenExpression
File:
/opt/code/velox4j/src/main/cpp/build/_deps/velox-src/velox/expression/ExprCompiler.cpp
Line: 477
Stack trace:
# 0 facebook::velox::VeloxException::VeloxException(char const*, unsigned
long, char const*, std::basic_string_view<char, std::char_traits<char> >,
std::basic_string_view<char, std::char_traits<char> >,
std::basic_string_view<char, std::char_traits<char> >,
std::basic_string_view<char, std::char_traits<char> >, bool,
facebook::velox::VeloxException::Type, std::basic_string_view<char,
std::char_traits<char> >)
# 1 void
facebook::velox::detail::veloxCheckFail<facebook::velox::VeloxUserError,
std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >
const&>(facebook::velox::detail::VeloxCheckFailArgs const&,
std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >
const&)
# 2 facebook::velox::exec::(anonymous
namespace)::compileRewrittenExpression(std::shared_ptr<facebook::velox::core::ITypedExpr
const> const&, facebook::velox::exec::(anonymous namespace)::Scope*,
facebook::velox::core::QueryConfig const&,
facebook::velox::memory::MemoryPool*,
std::unordered_set<std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char> >, std::hash<std::__cxx11::basic_string<char,
std::char_traits<char>, std::allocator<char> > >,
std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char> > >, std::allocator<std::__cxx11::basic_string<char,
std::char_traits<char>, std::allocator<char> > > > const&, bool)
# 3 facebook::velox::exec::(anonymous
namespace)::compileExpression(std::shared_ptr<facebook::velox::core::ITypedExpr
const> const&, facebook::velox::exec::(anonymous namespace)::Scope*,
facebook::velox::core::QueryConfig const&,
facebook::velox::memory::MemoryPool*,
std::unordered_set<std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char> >, std::hash<std::__cxx11::basic_string<char,
std::char_traits<char>, std::allocator<char> > >,
std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char> > >, std::allocator<std::__cxx11::basic_string<char,
std::char_traits<char>, std::allocator<char> > > > const&, bool)
# 4
facebook::velox::exec::compileExpressions(std::vector<std::shared_ptr<facebook::velox::core::ITypedExpr
const>, std::allocator<std::shared_ptr<facebook::velox::core::ITypedExpr
const> > > const&, facebook::velox::core::ExecCtx*,
facebook::velox::exec::ExprSet*, bool)
# 5
facebook::velox::exec::ExprSet::ExprSet(std::vector<std::shared_ptr<facebook::velox::core::ITypedExpr
const>, std::allocator<std::shared_ptr<facebook::velox::core::ITypedExpr
const> > > const&, facebook::velox::core::ExecCtx*, bool)
# 6
facebook::velox::exec::SimpleExpressionEvaluator::compile(std::shared_ptr<facebook::velox::core::ITypedExpr
const> const&)
# 7 velox4j::Evaluator::Evaluator(velox4j::MemoryManager*,
std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)
# 8 velox4j::(anonymous namespace)::createEvaluator(JNIEnv_*, _jobject*,
_jstring*)
# 9 0x0000e3b7c460bcfc
at io.github.zhztheplayer.velox4j.jni.JniWrapper.createEvaluator(Native
Method)
at
io.github.zhztheplayer.velox4j.jni.JniApi.createEvaluator(JniApi.java:64)
at
io.github.zhztheplayer.velox4j.eval.Evaluations.createEvaluator(Evaluations.java:29)
at
io.github.zhztheplayer.velox4j.eval.EvaluationTest.testMultiply(EvaluationTest.java:116)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at
org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at
org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
at
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
at
org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
at
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at
com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
at
com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
at
com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
at
com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
at
com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:232)
at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:55)
```
Will this kind of error message be sufficient for Gluten Flink?
But I just realized the native lib may not be loaded in Flink client side so
I have no idea whether this approach is feasible at the moment.
--
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]
