[GLUTEN-5953][VL] Prevent pushdown filters with unsupported data types to scan node #5954

WangGuangxin · 2024-06-02T13:29:22Z

What changes were proposed in this pull request?

Prevent pushdown filters with unsupported data types to scan node in case the scan fallback to vanilla scan operator

(Fixes: #5953)

How was this patch tested?

UT

github-actions · 2024-06-02T13:29:41Z

#5953

FelixYBW · 2024-06-03T20:24:46Z

Do you mean these fields are not supported in filter pushdown?

              StructField("short_decimal_field", DecimalType(5, 2), true),
              StructField("long_decimal_field", DecimalType(32, 8), true),
              StructField("binary_field", BinaryType, true),
              StructField("timestamp_field", TimestampType, true)

WangGuangxin · 2024-06-04T00:14:03Z

Do you mean these fields are not supported in filter pushdown?

              StructField("short_decimal_field", DecimalType(5, 2), true),
              StructField("long_decimal_field", DecimalType(32, 8), true),
              StructField("binary_field", BinaryType, true),
              StructField("timestamp_field", TimestampType, true)

@FelixYBW Before this PR, Gluten will try to pushdown them, but throw Subfield filters creation not supported for input type **, which make FileSourceScan fallback to Vanilla Spark.
After this PR, Gluten will not try to pushdown them.

BTW, short_decimal_field is supported to pushdown, the other three fields don't.

github-actions · 2024-06-23T09:24:23Z

Run Gluten Clickhouse CI

github-actions · 2024-06-23T09:24:48Z

Run Gluten Clickhouse CI

WangGuangxin · 2024-06-24T02:47:27Z

cc @PHILO-HE @rui-mo can you help review this?

rui-mo · 2024-06-24T12:45:05Z

backends-velox/src/test/scala/org/apache/gluten/execution/VeloxScanSuite.scala

+            """select * from t where binary_field = '3.14'""".stripMargin
+          )(checkGlutenOperatorMatch[FileSourceScanExecTransformer])
+          runQueryAndCompare(
+            """select * from t where timestamp_field = current_timestamp()""".stripMargin


We have made some updates on the timestamp support. Could you help check if the pushdown of timestamp is supported?

Yes, I think it's already supported in velox side. But as for now, Gluten can still fallback since mapToFilters doesn't support yet. I think we can support it in next PR.

Got it. Would you like to open an issue to track its support? Thanks.

here is the issue #6642

cpp/velox/substrait/SubstraitToVeloxPlan.h

github-actions · 2024-07-21T07:56:49Z

Run Gluten Clickhouse CI

PHILO-HE · 2024-07-23T02:46:36Z

@kecookier, @rui-mo

jiangjiangtian · 2024-07-23T11:59:35Z

I think we should also prevent pushdown filters from singularOrLists.
https://github.com/apache/incubator-gluten/blob/main/cpp/velox/substrait/SubstraitToVeloxPlan.cc#L1694

WangGuangxin · 2024-07-29T14:57:35Z

I think we should also prevent pushdown filters from singularOrLists. https://github.com/apache/incubator-gluten/blob/main/cpp/velox/substrait/SubstraitToVeloxPlan.cc#L1694

@jiangjiangtian maybe yes, but since there is already type check, I'm not quite sure. https://github.com/apache/incubator-gluten/blob/main/cpp/velox/substrait/SubstraitToVeloxPlan.cc#L2469 . I think it can be in another PR if needed?

rui-mo · 2024-07-31T01:35:56Z

cpp/velox/substrait/SubstraitToVeloxPlan.cc

+    case TypeKind::BOOLEAN:
+    case TypeKind::VARCHAR:
+    case TypeKind::ARRAY:
+    case TypeKind::MAP:


It looks like most type are supported. Would you list the unsupported types instead?

rui-mo · 2024-07-31T01:37:34Z

cpp/velox/substrait/SubstraitToVeloxPlan.h

+  static bool isPushdownSupported(TypePtr inputType);
+
+  /// Check whether the scalar function contains data type that doesn't to pushdown.
+  static bool canPushdownScalarFunction(


Could we merge it with canPushdownFunction which checks if a scalar function can be pushed down?

jiangjiangtian · 2024-07-31T02:56:30Z

I think we should also prevent pushdown filters from singularOrLists. https://github.com/apache/incubator-gluten/blob/main/cpp/velox/substrait/SubstraitToVeloxPlan.cc#L1694

@jiangjiangtian maybe yes, but since there is already type check, I'm not quite sure. https://github.com/apache/incubator-gluten/blob/main/cpp/velox/substrait/SubstraitToVeloxPlan.cc#L2469 . I think it can be in another PR if needed?

Yes, I think you are right. There is already type check.

github-actions · 2024-08-02T13:50:28Z

Run Gluten Clickhouse CI

github-actions · 2024-08-02T14:29:35Z

Run Gluten Clickhouse CI

github-actions · 2024-08-03T01:44:32Z

Run Gluten Clickhouse CI

github-actions · 2024-08-06T12:30:52Z

Run Gluten Clickhouse CI

github-actions · 2024-08-06T13:51:40Z

Run Gluten Clickhouse CI

rui-mo · 2024-08-07T08:37:29Z

cpp/velox/substrait/SubstraitToVeloxPlan.cc

@@ -1712,19 +1733,6 @@ void SubstraitToVeloxPlanConverter::separateFilters(
  for (const auto& scalarFunction : scalarFunctions) {
    auto filterNameSpec = SubstraitParser::findFunctionSpec(functionMap_, scalarFunction.function_reference());
    auto filterName = SubstraitParser::getNameBeforeDelimiter(filterNameSpec);
-    // Add all decimal filters to remaining functions because their pushdown are not supported.
-    if (format == dwio::common::FileFormat::ORC && scalarFunction.arguments().size() > 0) {


Looks like previous logic only excludes decimal type for ORC format, but now we are excluding HUGEINT for all file formats. Could you confirm if it is expected?

The ORC format already supports short-decimal predicate pushdown. @jiangjiangtian Can you help test this?

I verify that short-decimal predicate can be pushdown, as follows:

The main idea of these three types (Timestamp/VarBinary/HugeInt) in this PR is that if we don't put them in remainingFilter, then when we call mapToFilters, a exception Subfield filters creation not supported for input type '{}' in mapToFilters will throw, which will cause scan fallback.

The reason why mapToFilters don't support these three types is because there is no MultiRangeType or related type traits defined, which has nothing to do with the fileformat. cc @rui-mo @jiangjiangtian @kecookier

That means actually we don't support the pushdown of long decimal in Parquet and ORC. Is that correct?

That means actually we don't support the pushdown of long decimal in Parquet and ORC. Is that correct?

@rui-mo Currently yes. But to support a HugeintMultiRange in velox side is not a big issue, I can work on it later.

cpp/velox/substrait/SubstraitToVeloxPlan.cc

github-actions · 2024-08-14T07:19:28Z

Run Gluten Clickhouse CI

github-actions · 2024-08-14T07:24:00Z

Run Gluten Clickhouse CI

github-actions · 2024-08-14T07:37:44Z

Run Gluten Clickhouse CI

rui-mo · 2024-08-15T01:15:18Z

cpp/velox/substrait/SubstraitToVeloxPlan.cc

-  return canPushdown;
+
+  // Check whether data type is supported or not
+  if (!veloxTypeList.empty() && fieldIdx < veloxTypeList.size() && !isPushdownSupported(veloxTypeList.at(fieldIdx))) {


Is it a bug if 'fieldIdx >= veloxTypeList.size()'? Shall we just add a check to ensure it does not happen?

github-actions · 2024-09-30T02:00:18Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions · 2024-10-11T01:57:48Z

This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks.

WangGuangxin force-pushed the fix_pushdown branch from e870cd1 to 65614a1 Compare June 23, 2024 09:24

rui-mo reviewed Jun 24, 2024

View reviewed changes

WangGuangxin force-pushed the fix_pushdown branch from 65614a1 to 5ba1899 Compare July 21, 2024 07:56

PHILO-HE mentioned this pull request Jul 22, 2024

[VL] Do not pushdown filters with HUGEINT columns #6464

Closed

rui-mo reviewed Jul 31, 2024

View reviewed changes

github-actions bot added CORE works for Gluten Core VELOX labels Aug 2, 2024

WangGuangxin force-pushed the fix_pushdown branch from 8c36b3f to 4afd05e Compare August 3, 2024 01:44

WangGuangxin force-pushed the fix_pushdown branch from 4afd05e to a6f57c7 Compare August 6, 2024 12:30

WangGuangxin requested a review from rui-mo August 6, 2024 14:45

rui-mo reviewed Aug 7, 2024

View reviewed changes

cpp/velox/substrait/SubstraitToVeloxPlan.cc Outdated Show resolved Hide resolved

pushdown datatype check

19668f1

WangGuangxin added 7 commits August 14, 2024 15:23

fix style

76ba8ab

fix ut

8006e67

fix

d176862

address comments

588c539

fix

c055147

fix style

ad927fe

address comments

df3eb3f

WangGuangxin force-pushed the fix_pushdown branch from d796a8c to df3eb3f Compare August 14, 2024 07:23

fix style

4c6ebc7

rui-mo reviewed Aug 15, 2024

View reviewed changes

github-actions bot added the stale stale label Sep 30, 2024

github-actions bot closed this Oct 11, 2024

rui-mo mentioned this pull request Nov 14, 2024

[VL] Add test for scan operator with filter on decimal/timestamp/binary field #7945

Merged

[GLUTEN-5953][VL] Prevent pushdown filters with unsupported data types to scan node #5954

[GLUTEN-5953][VL] Prevent pushdown filters with unsupported data types to scan node #5954

Conversation

WangGuangxin commented Jun 2, 2024

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Jun 2, 2024

FelixYBW commented Jun 3, 2024

WangGuangxin commented Jun 4, 2024

github-actions bot commented Jun 23, 2024

github-actions bot commented Jun 23, 2024

WangGuangxin commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 21, 2024

PHILO-HE commented Jul 23, 2024

jiangjiangtian commented Jul 23, 2024

WangGuangxin commented Jul 29, 2024

Choose a reason for hiding this comment

rui-mo Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

jiangjiangtian commented Jul 31, 2024

github-actions bot commented Aug 2, 2024

github-actions bot commented Aug 2, 2024

github-actions bot commented Aug 3, 2024

github-actions bot commented Aug 6, 2024

github-actions bot commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 14, 2024

github-actions bot commented Aug 14, 2024

github-actions bot commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 30, 2024

github-actions bot commented Oct 11, 2024

rui-mo Jul 31, 2024 •

edited

Loading