address comments

apache · Aug 6, 2024 · 0d85569 · 0d85569
1 parent 0993730
commit 0d85569
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 55 deletions.
diff --git a/cpp/velox/substrait/SubstraitToVeloxPlan.cc b/cpp/velox/substrait/SubstraitToVeloxPlan.cc
@@ -1600,17 +1600,28 @@ bool SubstraitToVeloxPlanConverter::childrenFunctionsOnSameField(
 bool SubstraitToVeloxPlanConverter::canPushdownFunction(
     const ::substrait::Expression_ScalarFunction& scalarFunction,
     const std::string& filterName,
-    uint32_t& fieldIdx) {
-  // Condtions can be pushed down.
+    uint32_t& fieldIdx,
+    const std::vector<TypePtr>& veloxTypeList) {
+  // Conditions can be pushed down.
   static const std::unordered_set<std::string> supportedFunctions = {sIsNotNull, sIsNull, sGte, sGt, sLte, sLt, sEqual};
 
-  bool canPushdown = false;
-  if (supportedFunctions.find(filterName) != supportedFunctions.end() &&
-      fieldOrWithLiteral(scalarFunction.arguments(), fieldIdx)) {
-    // The arg should be field or field with literal.
-    canPushdown = true;
+  if (supportedFunctions.find(filterName) == supportedFunctions.end()) {
+    return false;
+  }
+
+  // The arg should be field or field with literal.
+  if(!fieldOrWithLiteral(scalarFunction.arguments(), fieldIdx)) {
+    return false;
   }
-  return canPushdown;
+
+  // check whether data type is supported or not
+  if (!veloxTypeList.empty() &&
+      fieldIdx < veloxTypeList.size() &&
+      !isPushdownSupported(veloxTypeList.at(fieldIdx))) {
+    return false;
+  }
+
+  return true;
 }
 
 bool SubstraitToVeloxPlanConverter::canPushdownNot(
@@ -1688,46 +1699,16 @@ bool SubstraitToVeloxPlanConverter::canPushdownOr(
 
 bool SubstraitToVeloxPlanConverter::isPushdownSupported(TypePtr inputType) {
   // keep the same with mapToFilters
-  if (inputType->isDate()) {
-    return true;
-  }
   switch (inputType->kind()) {
-    case TypeKind::TINYINT:
-    case TypeKind::SMALLINT:
-    case TypeKind::INTEGER:
-    case TypeKind::BIGINT:
-    case TypeKind::REAL:
-    case TypeKind::DOUBLE:
-    case TypeKind::BOOLEAN:
-    case TypeKind::VARCHAR:
-    case TypeKind::ARRAY:
-    case TypeKind::MAP:
-      return true;
+    case TypeKind::TIMESTAMP:
+    case TypeKind::VARBINARY:
+    case TypeKind::HUGEINT:
+      return false;
     default:
       return false;
   }
 }
 
-bool SubstraitToVeloxPlanConverter::canPushdownScalarFunction(
-    const ::substrait::Expression_ScalarFunction& function,
-    const std::vector<TypePtr>& veloxTypeList) {
-  for (const auto& arg : function.arguments()) {
-    if (arg.value().has_scalar_function()) {
-      const auto& scalarFunction = arg.value().scalar_function();
-      if (!canPushdownScalarFunction(scalarFunction, veloxTypeList)) {
-        return false;
-      }
-    } else if (arg.value().has_selection()) {
-      auto value = arg.value();
-      uint32_t fieldIndex = SubstraitParser::parseReferenceSegment(value.selection().direct_reference());
-      if (!veloxTypeList.empty() && !isPushdownSupported(veloxTypeList.at(fieldIndex))) {
-        return false;
-      }
-    }
-  }
-  return true;
-}
-
 void SubstraitToVeloxPlanConverter::separateFilters(
     std::vector<RangeRecorder>& rangeRecorders,
     const std::vector<::substrait::Expression_ScalarFunction>& scalarFunctions,
@@ -1754,12 +1735,6 @@ void SubstraitToVeloxPlanConverter::separateFilters(
   for (const auto& scalarFunction : scalarFunctions) {
     auto filterNameSpec = SubstraitParser::findFunctionSpec(functionMap_, scalarFunction.function_reference());
     auto filterName = SubstraitParser::getNameBeforeDelimiter(filterNameSpec);
-    // Add filters to remaining functions if datatype is not supported.
-    if (!canPushdownScalarFunction(scalarFunction, veloxTypeList)) {
-      remainingFunctions.emplace_back(scalarFunction);
-      continue;
-    }
-
     // Check whether NOT and OR functions can be pushed down.
     // If yes, the scalar function will be added into the subfield functions.
     if (filterName == sNot) {
@@ -1777,7 +1752,7 @@ void SubstraitToVeloxPlanConverter::separateFilters(
     } else {
       // Check if the condition is supported to be pushed down.
       uint32_t fieldIdx;
-      if (canPushdownFunction(scalarFunction, filterName, fieldIdx) &&
+      if (canPushdownFunction(scalarFunction, filterName, fieldIdx, veloxTypeList) &&
           rangeRecorders.at(fieldIdx).setCertainRangeForFunction(filterName)) {
         subfieldFunctions.emplace_back(scalarFunction);
       } else {

diff --git a/cpp/velox/substrait/SubstraitToVeloxPlan.h b/cpp/velox/substrait/SubstraitToVeloxPlan.h
@@ -451,7 +451,8 @@ class SubstraitToVeloxPlanConverter {
   static bool canPushdownFunction(
       const ::substrait::Expression_ScalarFunction& scalarFunction,
       const std::string& filterName,
-      uint32_t& fieldIdx);
+      uint32_t& fieldIdx,
+      const std::vector<TypePtr>& veloxTypeList);
 
   /// Returns whether a NOT function can be pushed down.
   bool canPushdownNot(
@@ -476,11 +477,6 @@ class SubstraitToVeloxPlanConverter {
   /// Check whether the data type is supported to pushdown.
   static bool isPushdownSupported(TypePtr inputType);
 
-  /// Check whether the scalar function contains data type that doesn't to pushdown.
-  static bool canPushdownScalarFunction(
-      const ::substrait::Expression_ScalarFunction& scalarFunction,
-      const std::vector<TypePtr>& veloxTypeList);
-
   /// Extract the scalar function, and set the filter info for different types
   /// of columns. If reverse is true, the opposite filter info will be set.
   void setFilterInfo(