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-unsubscr...@gluten.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@gluten.apache.org
For additional commands, e-mail: commits-h...@gluten.apache.org

Reply via email to