lgbo-ustc commented on code in PR #10047:
URL:
https://github.com/apache/incubator-gluten/pull/10047#discussion_r2163793005
##########
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:
The message is used to help identify why a suitable converter was not
found. But I think we should probably focus more on discussing the rationale
behind allowing multiple convertors to be bound to the same operator.
We have encountered some cases where operators in Flink have the same name
but, due to different input parameter types, they map to different functions in
Velox. We could simply bind only one converter to each operator, and let the
converter determine the actual Velox function internally. However, I think this
approach would lead to some code duplication. That's why we designed the system
to allow binding multiple converters to a single operator. In most cases, I
believe a single converter with doValidation as the default implementation
(which simply returns ok) is sufficient.
--
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]
