[GLUTEN-8742][VL] Improve the cast validation logic on native side #8743
Conversation
ArnavBalyan
force-pushed
the
arnavb/refactor-castvalidation
branch
from
c924206 to
5c5b95e
Compare
|
cc @PHILO-HE could you please review UTs are now green thanks |
| @@ -259,6 +259,57 @@ bool SubstraitToVeloxPlanValidator::validateLiteral( | |||
| return true; | |||
| } | |||
|
|
|||
| bool SubstraitToVeloxPlanValidator::isDeniedCast( | |||
There was a problem hiding this comment.
Preferred function name: isSupportedCast. Then return false for unsupported case.
|
|
||
| if (toType->isIntervalYearMonth()) { | ||
| LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported."); | ||
| if (SubstraitToVeloxPlanValidator::isDeniedCast(fromKind, toKind, input->type(), toType)) { |
There was a problem hiding this comment.
return isSuppportedCast(xxx);
There was a problem hiding this comment.
Since we can easily get fromKind & toKind from the other arguments (see below), can we remove them from the argument list?
auto fromKind = input->type()->kind();
auto toKind = toType->kind();
ArnavBalyan
force-pushed
the
arnavb/refactor-castvalidation
branch
2 times, most recently
from
c5e6cad to
29d503d
Compare
ArnavBalyan
force-pushed
the
arnavb/refactor-castvalidation
branch
from
29d503d to
bc5def9
Compare
There was a problem hiding this comment.
@ArnavBalyan, please fix code format violation, as reported by CI.
| auto fromKind = input->type()->kind(); | ||
| auto toKind = toType->kind(); | ||
|
|
||
| if (SubstraitToVeloxPlanValidator::isAllowedCast(input->type(), toType)) { |
There was a problem hiding this comment.
Remove SubstraitToVeloxPlanValidator::, as it is not necessary.
For simplicity:
return isAllowedCast();
There was a problem hiding this comment.
Done formatting, isAllowedCast is using LOG_VALIDATION_MSG, etc which are scoped to SubstraitToVeloxPlanValidator, let me know if there is an alternate way thanks
There was a problem hiding this comment.
@ArnavBalyan, I tried with return isAllowedCast(input->type(), toType), it works. Could you change this code and let's see CI feedback?
There was a problem hiding this comment.
I see some test result here https://github.com/apache/incubator-gluten/actions/runs/13651331134/job/38160388055?pr=8743
There was a problem hiding this comment.
@ArnavBalyan, I guess different GCC version has different requirement. I tested with lower GCC version. Sorry for this. Please add the namespace to make it available in this scope.
There was a problem hiding this comment.
No worries at all thanks for the review, updated it
|
cc @PHILO-HE could you please take a look thanks! |
| core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType); | ||
|
|
||
| auto fromKind = input->type()->kind(); | ||
| auto toKind = toType->kind(); |
There was a problem hiding this comment.
Please remove these two useless lines.
@ArnavBalyan, basically looks good! Just two trivial comments. Please also rebase the code. Thank you! |
| const auto& toType = SubstraitParser::parseType(castExpr.type()); | ||
| core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType); | ||
| // Limited support for DATE to X. | ||
| if (fromType->isDate() && toKind != TypeKind::TIMESTAMP && toKind != TypeKind::VARCHAR) { |
There was a problem hiding this comment.
nit: Maybe we can refactor it into a coding style similar to Velox, which would make the code more readable.
There was a problem hiding this comment.
Yes agreed, currently we don't have as many cast conditions, I am adding some more casts, I'll explore this once the new casts are added
ArnavBalyan
force-pushed
the
arnavb/refactor-castvalidation
branch
2 times, most recently
from
96715f0 to
9edb0d8
Compare
ArnavBalyan
force-pushed
the
arnavb/refactor-castvalidation
branch
from
9edb0d8 to
e0c1519
Compare
PHILO-HE
changed the title
What changes were proposed in this pull request?
How was this patch tested?