You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "ArnavBalyan (via GitHub)" <gi...@apache.org> on 2025/02/16 15:27:15 UTC
[PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
ArnavBalyan opened a new pull request, #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743
## What changes were proposed in this pull request?
- Currently the cast validation is unreadable and hard to maintain due to various addition/removals over time.
- Refactor the validator to make cast validations consolidated and maintainable for new cast supports.
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
- Existing UTs
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "jackylee-ch (via GitHub)" <gi...@apache.org>.
jackylee-ch commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1978887900
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,69 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(const TypePtr& fromType, const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
Review Comment:
nit: Maybe we can refactor it into a coding style similar to [Velox](https://github.com/facebookincubator/velox/blob/main/velox/expression/CastExpr.cpp#L752), which would make the code more readable.
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,69 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(const TypePtr& fromType, const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
Review Comment:
Limited support
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#issuecomment-2694535442
cc @PHILO-HE could you please take a look thanks!
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1961228656
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -269,46 +320,13 @@ bool SubstraitToVeloxPlanValidator::validateCast(
const auto& toType = SubstraitParser::parseType(castExpr.type());
core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
- LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
- return false;
- }
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ if (SubstraitToVeloxPlanValidator::isDeniedCast(fromKind, toKind, input->type(), toType)) {
Review 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();
```
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1974954481
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,78 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(
+ const TypePtr& fromType,
+ const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
+ if (toKind == TypeKind::TIMESTAMP && !fromType->isDate()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " to TIMESTAMP is not supported.");
return false;
}
- // Casting from some types is not supported. See CastExpr::applyPeeled.
- if (input->type()->isDate()) {
- // Only support cast date to varchar & timestamp
- if (toType->kind() != TypeKind::VARCHAR && toType->kind() != TypeKind::TIMESTAMP) {
- LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
- return false;
- }
- } else if (input->type()->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting from INTERVAL_YEAR_MONTH is not supported.");
+ // Limited support for Complex types.
+ if (complexTypeList.find(fromKind) != complexTypeList.end()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " is not currently supported.");
return false;
}
- switch (input->type()->kind()) {
- case TypeKind::ARRAY:
- case TypeKind::MAP:
- case TypeKind::ROW:
- case TypeKind::VARBINARY:
- LOG_VALIDATION_MSG("Invalid input type in casting: ARRAY/MAP/ROW/VARBINARY.");
- return false;
- case TypeKind::TIMESTAMP:
- // Only support casting timestamp to date or varchar.
- if (!toType->isDate() && toType->kind() != TypeKind::VARCHAR) {
- LOG_VALIDATION_MSG(
- "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
- return false;
- }
- default: {
- }
- }
+
return true;
}
+bool SubstraitToVeloxPlanValidator::validateCast(
+ const ::substrait::Expression::Cast& castExpr,
+ const RowTypePtr& inputType) {
+ if (!validateExpression(castExpr.input(), inputType)) {
+ return false;
+ }
+
+ const auto& toType = SubstraitParser::parseType(castExpr.type());
+ core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
+
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
+
+ if (SubstraitToVeloxPlanValidator::isAllowedCast(input->type(), toType)) {
Review 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
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#issuecomment-2696521288
> cc @PHILO-HE could you please take a look thanks!
@ArnavBalyan, basically looks good! Just two trivial comments. Please also rebase the code. Thank you!
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#issuecomment-2664814268
cc @PHILO-HE could you please review UTs are now green thanks
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the cast validation logic on native side [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE merged PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1978813802
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,78 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(
+ const TypePtr& fromType,
+ const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
+ if (toKind == TypeKind::TIMESTAMP && !fromType->isDate()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " to TIMESTAMP is not supported.");
return false;
}
- // Casting from some types is not supported. See CastExpr::applyPeeled.
- if (input->type()->isDate()) {
- // Only support cast date to varchar & timestamp
- if (toType->kind() != TypeKind::VARCHAR && toType->kind() != TypeKind::TIMESTAMP) {
- LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
- return false;
- }
- } else if (input->type()->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting from INTERVAL_YEAR_MONTH is not supported.");
+ // Limited support for Complex types.
+ if (complexTypeList.find(fromKind) != complexTypeList.end()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " is not currently supported.");
return false;
}
- switch (input->type()->kind()) {
- case TypeKind::ARRAY:
- case TypeKind::MAP:
- case TypeKind::ROW:
- case TypeKind::VARBINARY:
- LOG_VALIDATION_MSG("Invalid input type in casting: ARRAY/MAP/ROW/VARBINARY.");
- return false;
- case TypeKind::TIMESTAMP:
- // Only support casting timestamp to date or varchar.
- if (!toType->isDate() && toType->kind() != TypeKind::VARCHAR) {
- LOG_VALIDATION_MSG(
- "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
- return false;
- }
- default: {
- }
- }
+
return true;
}
+bool SubstraitToVeloxPlanValidator::validateCast(
+ const ::substrait::Expression::Cast& castExpr,
+ const RowTypePtr& inputType) {
+ if (!validateExpression(castExpr.input(), inputType)) {
+ return false;
+ }
+
+ const auto& toType = SubstraitParser::parseType(castExpr.type());
+ core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
+
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
+
+ if (SubstraitToVeloxPlanValidator::isAllowedCast(input->type(), toType)) {
Review Comment:
@ArnavBalyan, I tried with `return isAllowedCast(input->type(), toType)`, it works. Could you change this code and let's see CI feedback?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1979298417
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,78 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(
+ const TypePtr& fromType,
+ const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
+ if (toKind == TypeKind::TIMESTAMP && !fromType->isDate()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " to TIMESTAMP is not supported.");
return false;
}
- // Casting from some types is not supported. See CastExpr::applyPeeled.
- if (input->type()->isDate()) {
- // Only support cast date to varchar & timestamp
- if (toType->kind() != TypeKind::VARCHAR && toType->kind() != TypeKind::TIMESTAMP) {
- LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
- return false;
- }
- } else if (input->type()->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting from INTERVAL_YEAR_MONTH is not supported.");
+ // Limited support for Complex types.
+ if (complexTypeList.find(fromKind) != complexTypeList.end()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " is not currently supported.");
return false;
}
- switch (input->type()->kind()) {
- case TypeKind::ARRAY:
- case TypeKind::MAP:
- case TypeKind::ROW:
- case TypeKind::VARBINARY:
- LOG_VALIDATION_MSG("Invalid input type in casting: ARRAY/MAP/ROW/VARBINARY.");
- return false;
- case TypeKind::TIMESTAMP:
- // Only support casting timestamp to date or varchar.
- if (!toType->isDate() && toType->kind() != TypeKind::VARCHAR) {
- LOG_VALIDATION_MSG(
- "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
- return false;
- }
- default: {
- }
- }
+
return true;
}
+bool SubstraitToVeloxPlanValidator::validateCast(
+ const ::substrait::Expression::Cast& castExpr,
+ const RowTypePtr& inputType) {
+ if (!validateExpression(castExpr.input(), inputType)) {
+ return false;
+ }
+
+ const auto& toType = SubstraitParser::parseType(castExpr.type());
+ core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
+
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
+
+ if (SubstraitToVeloxPlanValidator::isAllowedCast(input->type(), toType)) {
Review 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.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1961193627
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -259,6 +259,57 @@ bool SubstraitToVeloxPlanValidator::validateLiteral(
return true;
}
+bool SubstraitToVeloxPlanValidator::isDeniedCast(
Review Comment:
Preferred function name: `isSupportedCast`. Then return false for unsupported case.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1978858451
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,78 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(
+ const TypePtr& fromType,
+ const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
+ if (toKind == TypeKind::TIMESTAMP && !fromType->isDate()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " to TIMESTAMP is not supported.");
return false;
}
- // Casting from some types is not supported. See CastExpr::applyPeeled.
- if (input->type()->isDate()) {
- // Only support cast date to varchar & timestamp
- if (toType->kind() != TypeKind::VARCHAR && toType->kind() != TypeKind::TIMESTAMP) {
- LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
- return false;
- }
- } else if (input->type()->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting from INTERVAL_YEAR_MONTH is not supported.");
+ // Limited support for Complex types.
+ if (complexTypeList.find(fromKind) != complexTypeList.end()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " is not currently supported.");
return false;
}
- switch (input->type()->kind()) {
- case TypeKind::ARRAY:
- case TypeKind::MAP:
- case TypeKind::ROW:
- case TypeKind::VARBINARY:
- LOG_VALIDATION_MSG("Invalid input type in casting: ARRAY/MAP/ROW/VARBINARY.");
- return false;
- case TypeKind::TIMESTAMP:
- // Only support casting timestamp to date or varchar.
- if (!toType->isDate() && toType->kind() != TypeKind::VARCHAR) {
- LOG_VALIDATION_MSG(
- "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
- return false;
- }
- default: {
- }
- }
+
return true;
}
+bool SubstraitToVeloxPlanValidator::validateCast(
+ const ::substrait::Expression::Cast& castExpr,
+ const RowTypePtr& inputType) {
+ if (!validateExpression(castExpr.input(), inputType)) {
+ return false;
+ }
+
+ const auto& toType = SubstraitParser::parseType(castExpr.type());
+ core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
+
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
+
+ if (SubstraitToVeloxPlanValidator::isAllowedCast(input->type(), toType)) {
Review Comment:
updated
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,76 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(const TypePtr& fromType, const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
+ if (toKind == TypeKind::TIMESTAMP && !fromType->isDate()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " to TIMESTAMP is not supported.");
return false;
}
- // Casting from some types is not supported. See CastExpr::applyPeeled.
- if (input->type()->isDate()) {
- // Only support cast date to varchar & timestamp
- if (toType->kind() != TypeKind::VARCHAR && toType->kind() != TypeKind::TIMESTAMP) {
- LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
- return false;
- }
- } else if (input->type()->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting from INTERVAL_YEAR_MONTH is not supported.");
+ // Limited support for Complex types.
+ if (complexTypeList.find(fromKind) != complexTypeList.end()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " is not currently supported.");
return false;
}
- switch (input->type()->kind()) {
- case TypeKind::ARRAY:
- case TypeKind::MAP:
- case TypeKind::ROW:
- case TypeKind::VARBINARY:
- LOG_VALIDATION_MSG("Invalid input type in casting: ARRAY/MAP/ROW/VARBINARY.");
- return false;
- case TypeKind::TIMESTAMP:
- // Only support casting timestamp to date or varchar.
- if (!toType->isDate() && toType->kind() != TypeKind::VARCHAR) {
- LOG_VALIDATION_MSG(
- "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
- return false;
- }
- default: {
- }
- }
+
return true;
}
+bool SubstraitToVeloxPlanValidator::validateCast(
+ const ::substrait::Expression::Cast& castExpr,
+ const RowTypePtr& inputType) {
+ if (!validateExpression(castExpr.input(), inputType)) {
+ return false;
+ }
+
+ const auto& toType = SubstraitParser::parseType(castExpr.type());
+ core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
+
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
Review Comment:
updated
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1979008816
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,69 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(const TypePtr& fromType, const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
Review 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
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1961193627
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -259,6 +259,57 @@ bool SubstraitToVeloxPlanValidator::validateLiteral(
return true;
}
+bool SubstraitToVeloxPlanValidator::isDeniedCast(
Review Comment:
Preferred function name: `isSuppportedCast`. Then return false for unsupported case.
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -269,46 +320,13 @@ bool SubstraitToVeloxPlanValidator::validateCast(
const auto& toType = SubstraitParser::parseType(castExpr.type());
core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
- LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
- return false;
- }
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ if (SubstraitToVeloxPlanValidator::isDeniedCast(fromKind, toKind, input->type(), toType)) {
Review Comment:
`return isSuppportedCast(xxx);`
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1979206926
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,78 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(
+ const TypePtr& fromType,
+ const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
+ if (toKind == TypeKind::TIMESTAMP && !fromType->isDate()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " to TIMESTAMP is not supported.");
return false;
}
- // Casting from some types is not supported. See CastExpr::applyPeeled.
- if (input->type()->isDate()) {
- // Only support cast date to varchar & timestamp
- if (toType->kind() != TypeKind::VARCHAR && toType->kind() != TypeKind::TIMESTAMP) {
- LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
- return false;
- }
- } else if (input->type()->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting from INTERVAL_YEAR_MONTH is not supported.");
+ // Limited support for Complex types.
+ if (complexTypeList.find(fromKind) != complexTypeList.end()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " is not currently supported.");
return false;
}
- switch (input->type()->kind()) {
- case TypeKind::ARRAY:
- case TypeKind::MAP:
- case TypeKind::ROW:
- case TypeKind::VARBINARY:
- LOG_VALIDATION_MSG("Invalid input type in casting: ARRAY/MAP/ROW/VARBINARY.");
- return false;
- case TypeKind::TIMESTAMP:
- // Only support casting timestamp to date or varchar.
- if (!toType->isDate() && toType->kind() != TypeKind::VARCHAR) {
- LOG_VALIDATION_MSG(
- "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
- return false;
- }
- default: {
- }
- }
+
return true;
}
+bool SubstraitToVeloxPlanValidator::validateCast(
+ const ::substrait::Expression::Cast& castExpr,
+ const RowTypePtr& inputType) {
+ if (!validateExpression(castExpr.input(), inputType)) {
+ return false;
+ }
+
+ const auto& toType = SubstraitParser::parseType(castExpr.type());
+ core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
+
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
+
+ if (SubstraitToVeloxPlanValidator::isAllowedCast(input->type(), toType)) {
Review Comment:
I see some test result here https://github.com/apache/incubator-gluten/actions/runs/13651331134/job/38160388055?pr=8743
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "ArnavBalyan (via GitHub)" <gi...@apache.org>.
ArnavBalyan commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1979353258
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,78 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(
+ const TypePtr& fromType,
+ const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
+ if (toKind == TypeKind::TIMESTAMP && !fromType->isDate()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " to TIMESTAMP is not supported.");
return false;
}
- // Casting from some types is not supported. See CastExpr::applyPeeled.
- if (input->type()->isDate()) {
- // Only support cast date to varchar & timestamp
- if (toType->kind() != TypeKind::VARCHAR && toType->kind() != TypeKind::TIMESTAMP) {
- LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
- return false;
- }
- } else if (input->type()->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting from INTERVAL_YEAR_MONTH is not supported.");
+ // Limited support for Complex types.
+ if (complexTypeList.find(fromKind) != complexTypeList.end()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " is not currently supported.");
return false;
}
- switch (input->type()->kind()) {
- case TypeKind::ARRAY:
- case TypeKind::MAP:
- case TypeKind::ROW:
- case TypeKind::VARBINARY:
- LOG_VALIDATION_MSG("Invalid input type in casting: ARRAY/MAP/ROW/VARBINARY.");
- return false;
- case TypeKind::TIMESTAMP:
- // Only support casting timestamp to date or varchar.
- if (!toType->isDate() && toType->kind() != TypeKind::VARCHAR) {
- LOG_VALIDATION_MSG(
- "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
- return false;
- }
- default: {
- }
- }
+
return true;
}
+bool SubstraitToVeloxPlanValidator::validateCast(
+ const ::substrait::Expression::Cast& castExpr,
+ const RowTypePtr& inputType) {
+ if (!validateExpression(castExpr.input(), inputType)) {
+ return false;
+ }
+
+ const auto& toType = SubstraitParser::parseType(castExpr.type());
+ core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
+
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
+
+ if (SubstraitToVeloxPlanValidator::isAllowedCast(input->type(), toType)) {
Review Comment:
No worries at all thanks for the review, updated it
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1978816438
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,76 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(const TypePtr& fromType, const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
+ if (toKind == TypeKind::TIMESTAMP && !fromType->isDate()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " to TIMESTAMP is not supported.");
return false;
}
- // Casting from some types is not supported. See CastExpr::applyPeeled.
- if (input->type()->isDate()) {
- // Only support cast date to varchar & timestamp
- if (toType->kind() != TypeKind::VARCHAR && toType->kind() != TypeKind::TIMESTAMP) {
- LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
- return false;
- }
- } else if (input->type()->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting from INTERVAL_YEAR_MONTH is not supported.");
+ // Limited support for Complex types.
+ if (complexTypeList.find(fromKind) != complexTypeList.end()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " is not currently supported.");
return false;
}
- switch (input->type()->kind()) {
- case TypeKind::ARRAY:
- case TypeKind::MAP:
- case TypeKind::ROW:
- case TypeKind::VARBINARY:
- LOG_VALIDATION_MSG("Invalid input type in casting: ARRAY/MAP/ROW/VARBINARY.");
- return false;
- case TypeKind::TIMESTAMP:
- // Only support casting timestamp to date or varchar.
- if (!toType->isDate() && toType->kind() != TypeKind::VARCHAR) {
- LOG_VALIDATION_MSG(
- "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
- return false;
- }
- default: {
- }
- }
+
return true;
}
+bool SubstraitToVeloxPlanValidator::validateCast(
+ const ::substrait::Expression::Cast& castExpr,
+ const RowTypePtr& inputType) {
+ if (!validateExpression(castExpr.input(), inputType)) {
+ return false;
+ }
+
+ const auto& toType = SubstraitParser::parseType(castExpr.type());
+ core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
+
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
Review Comment:
Please remove these two useless lines.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on code in PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#discussion_r1967756312
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -240,59 +240,78 @@ bool SubstraitToVeloxPlanValidator::validateScalarFunction(
return true;
}
-bool SubstraitToVeloxPlanValidator::validateCast(
- const ::substrait::Expression::Cast& castExpr,
- const RowTypePtr& inputType) {
- if (!validateExpression(castExpr.input(), inputType)) {
+bool SubstraitToVeloxPlanValidator::isAllowedCast(
+ const TypePtr& fromType,
+ const TypePtr& toType) {
+ // Currently cast is not allowed for various categories, code has a bunch of rules
+ // which define the cast categories and if we should offload to velox. Currently
+ // the following categories are denied.
+ //
+ // 1. from/to isIntervalYearMonth is not allowed.
+ // 2. Date to most categories except few supported types is not allowed.
+ // 3. Timestamp to most categories except few supported types is not allowed.
+ // 4. Certain complex types are not allowed.
+
+ TypeKind fromKind = fromType->kind();
+ TypeKind toKind = toType->kind();
+
+ static const std::unordered_set<TypeKind> complexTypeList = {
+ TypeKind::ARRAY, TypeKind::MAP, TypeKind::ROW, TypeKind::VARBINARY};
+
+ // Don't support isIntervalYearMonth.
+ if (fromType->isIntervalYearMonth() || toType->isIntervalYearMonth()) {
+ LOG_VALIDATION_MSG("Casting involving INTERVAL_YEAR_MONTH is not supported.");
return false;
}
- 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) {
+ LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
+ return false;
+ }
- // Only support cast from date to timestamp
- if (toType->kind() == TypeKind::TIMESTAMP && !input->type()->isDate()) {
+ // Limited support for Timestamp to X.
+ if (fromKind == TypeKind::TIMESTAMP && !(toType->isDate() || toKind == TypeKind::VARCHAR)) {
LOG_VALIDATION_MSG(
- "Casting from " + input->type()->toString() + " to " + toType->toString() + " is not supported.");
+ "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
return false;
}
- if (toType->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting to " + toType->toString() + " is not supported.");
+ // Support for X to Timestamp.
+ if (toKind == TypeKind::TIMESTAMP && !fromType->isDate()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " to TIMESTAMP is not supported.");
return false;
}
- // Casting from some types is not supported. See CastExpr::applyPeeled.
- if (input->type()->isDate()) {
- // Only support cast date to varchar & timestamp
- if (toType->kind() != TypeKind::VARCHAR && toType->kind() != TypeKind::TIMESTAMP) {
- LOG_VALIDATION_MSG("Casting from DATE to " + toType->toString() + " is not supported.");
- return false;
- }
- } else if (input->type()->isIntervalYearMonth()) {
- LOG_VALIDATION_MSG("Casting from INTERVAL_YEAR_MONTH is not supported.");
+ // Limited support for Complex types.
+ if (complexTypeList.find(fromKind) != complexTypeList.end()) {
+ LOG_VALIDATION_MSG("Casting from " + fromType->toString() + " is not currently supported.");
return false;
}
- switch (input->type()->kind()) {
- case TypeKind::ARRAY:
- case TypeKind::MAP:
- case TypeKind::ROW:
- case TypeKind::VARBINARY:
- LOG_VALIDATION_MSG("Invalid input type in casting: ARRAY/MAP/ROW/VARBINARY.");
- return false;
- case TypeKind::TIMESTAMP:
- // Only support casting timestamp to date or varchar.
- if (!toType->isDate() && toType->kind() != TypeKind::VARCHAR) {
- LOG_VALIDATION_MSG(
- "Casting from TIMESTAMP to " + toType->toString() + " is not supported or has incorrect result.");
- return false;
- }
- default: {
- }
- }
+
return true;
}
+bool SubstraitToVeloxPlanValidator::validateCast(
+ const ::substrait::Expression::Cast& castExpr,
+ const RowTypePtr& inputType) {
+ if (!validateExpression(castExpr.input(), inputType)) {
+ return false;
+ }
+
+ const auto& toType = SubstraitParser::parseType(castExpr.type());
+ core::TypedExprPtr input = exprConverter_->toVeloxExpr(castExpr.input(), inputType);
+
+ auto fromKind = input->type()->kind();
+ auto toKind = toType->kind();
+
+ if (SubstraitToVeloxPlanValidator::isAllowedCast(input->type(), toType)) {
Review Comment:
Remove `SubstraitToVeloxPlanValidator::`, as it is not necessary.
For simplicity:
return isAllowedCast();
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [GLUTEN-8742][VL] Improve the castValidations in Velox to Substrait [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8743:
URL: https://github.com/apache/incubator-gluten/pull/8743#issuecomment-2661482883
https://github.com/apache/incubator-gluten/issues/8742
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org