[SPARK-49162][SQL] Push down date_trunc function #47666

IvanK-db · 2024-08-08T12:20:12Z

What changes were proposed in this pull request?

This PR:

Adds support for DATE_TRUNC in V2 optimization pushdown
Consumes this pushdown for Postgres & H2 Connectors

Why are the changes needed?

Performance improvements.

Does this PR introduce any user-facing change?

No

How was this patch tested?

New integration test in PostgresIntegrationSuite.

Was this patch authored or co-authored using generative AI tooling?

No

abhiips07 · 2024-08-09T06:09:52Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala

    "VAR_POP", "VAR_SAMP", "STDDEV_POP", "STDDEV_SAMP", "COVAR_POP", "COVAR_SAMP", "CORR",
    "REGR_INTERCEPT", "REGR_R2", "REGR_SLOPE", "REGR_SXY")
-  private val supportedFunctions = supportedAggregateFunctions
+  private val supportedDatetimeFunctions = Set("DATE_TRUNC")


private val supportedAggregateFunctions: Set[String] = Set( "MAX", "MIN", "SUM", "COUNT", "AVG", "VAR_POP", "VAR_SAMP", "STDDEV_POP", "STDDEV_SAMP", "COVAR_POP", "COVAR_SAMP", "CORR", "REGR_INTERCEPT", "REGR_R2", "REGR_SLOPE", "REGR_SXY", "DATE_TRUNC" )

This approach simplifies the code and keeps everything in one place. Let me know what you think!

Imo DATE_TRUNC should not be in the supportedAggregateFunctions as it's not an aggregate function

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala

HyukjinKwon · 2024-09-03T07:10:03Z

cc @beliefer

stefanbuk-db · 2024-09-03T13:18:06Z

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala

+        s"""
+            SELECT DATE_TRUNC('$format', time), COUNT(*)
+            | FROM $catalogName.datetime_table
+            | GROUP BY 1


Thanks for adding new tests! Can we do tests where DATE_TRUNC is a part of the predicate (where DATE_TRUNC = ....) to see if it works when we push down predicates. I am not sure if we do pushdowns in projection.

Added tests for that as well

Why introduce aggregate here? we just need test predicates.

The original query that was used to point out the function wasn't pushed down was similar to this one so i modeled the test case after it. Should we remove the aggregate test cases?

Yes. Let's remove them.

beliefer · 2024-09-04T12:43:58Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala

+  private val supportedDatetimeFunctions = Set("DATE_TRUNC")
+  private val supportedFunctions =
+    supportedAggregateFunctions ++
+      supportedDatetimeFunctions


Shall we remove supportedDatetimeFunctions by
private val supportedFunctions = supportedAggregateFunctions ++ Set("DATE_TRUNC") ?

My thinking here is that we might have other datetime functions pushed down in the future and this provides a convenient grouping for them, similar to existing supportedAggregateFunctions

The aggregate functions is very different from others(string functions, datetime functions and so on) and already defined a lot of aggregate functions.
We can define supportedDatetimeFunctions in future if need.

beliefer · 2024-09-04T12:45:15Z

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala

+    def testAggregatePushdown(format: String, expectedResult: Set[Row]): Unit = {
+      val df = sql(
+        s"""
+            SELECT DATE_TRUNC('$format', time), COUNT(*)


Is DATE_TRUNC supported by H2 database ?

https://www.h2database.com/html/functions.html#date_trunc

Looks like yes, however the original task was adding support only for Postgres for now.

I think we should support for H2 dialect first, then other database dialect.

Added pushdown for H2 as well

…SPARK-49162

beliefer · 2024-09-11T08:45:36Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala

    checkAnswer(df9, Seq(Row("alex")))
+
+    val df10 = sql("SELECT name FROM h2.test.datetime WHERE " +
+      "DATE_TRUNC('DAY', date1) = date'2022-05-19'")


Shall we add more test cases for other format supported by Spark, such as: YEAR, MM and so on.

Let's create a separate test case.

beliefer · 2024-09-11T08:53:28Z

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala

+        s"""
+            SELECT DATE_TRUNC('$format', time), COUNT(*)
+            | FROM $catalogName.datetime_table
+            | GROUP BY 1


Why introduce aggregate here? we just need test predicates.

beliefer · 2024-09-12T11:55:43Z

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala

+      val filters = df.queryExecution.optimizedPlan.collect {
+        case f: Filter => f
+      }
+      assert(filters.isEmpty)


Please reuse checkFilterPushed here.

IvanK-db · 2024-09-16T14:48:35Z

@beliefer Kind reminder, all comments have been addressed.

beliefer

LGTM. cc @cloud-fan Do you have any other comments?

beliefer · 2024-09-18T01:38:08Z

connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala

  }

-  private def checkFilterPushed(df: DataFrame, pushed: Boolean = true): Unit = {
+  protected def checkFilterPushed(df: DataFrame, pushed: Boolean = true): Unit = {


Could you rebase the repository? because the modifier already is protected.

milastdbx

Lets add more tests for date_trunc precisions.

Also, please update description a bit.
In this PR you are:

Adding support for DATE_TRUNC in V2 optimization pushdown
Consuming this pushdown for Postgres & H2 Connectors

milastdbx · 2024-09-18T09:25:24Z

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala

    }
  }
+
+  test("SPARK-49162: Push down filter date_trunc function") {


reading the documentation for pgsql:
https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-TRUNC
and databricks:
https://docs.databricks.com/en/sql/language-manual/functions/date_trunc.html

I see that they support microseconds while we support microsecond.

Can we test for all our supported precisions ? ALso can we try different casings ?

+1, let's test all the supported units.

Yes. Could we add other precisions for H2 and Postgres? @IvanK-db

cloud-fan · 2024-09-18T14:25:54Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala

    case _: DateAdd => generateExpressionWithName("DATE_ADD", expr, isPredicate)
    case _: DateDiff => generateExpressionWithName("DATE_DIFF", expr, isPredicate)
    case _: TruncDate => generateExpressionWithName("TRUNC", expr, isPredicate)
+    case _: TruncTimestamp => generateExpressionWithName("DATE_TRUNC", expr, isPredicate)


is DATE_TRUNC a standard SQL function or widely supported in the industry?

Not a standard function but it's supported by several databases - Postgres, Redshift, Snowflake, BQ

…SPARK-49162

beliefer · 2024-09-23T09:08:21Z

sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala

+    val expectedPlanFragment9 =
+      "PushedFilters: [(DATE_TRUNC('MicroseconD', TIME1)) = 1725560625000000]"
+    checkPushedInfo(df9, expectedPlanFragment9)
+    checkAnswer(df9, Seq(Row("adam")))


Please add test cases for all supported precisions.
Please add the negative test cases, such as: MicroSecondS.

Added all precisions. MicroSecondS seems to be a positive test case, looks to be an alias of microsecond so i added another negative test case.

beliefer

LGTM.

cloud-fan · 2024-10-08T14:15:58Z

My biggest concern is we translate the Spark TruncTimestamp expression to v2 DATE_TRUNC function. DATE_TRUNC is not in the SQL standard and personally I think it's more reasonable to let the v2 TRUNC function accept timestamp inputs. Redshift's TRUNC function also supports timestamp: https://docs.aws.amazon.com/redshift/latest/dg/r_TRUNC.html

also cc @srielau

github-actions · 2025-01-17T00:23:50Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

Push down date_trunc function

40dffcf

github-actions bot added the SQL label Aug 8, 2024

Import Filter

df5e590

abhiips07 reviewed Aug 9, 2024

View reviewed changes

stefanbuk-db reviewed Sep 2, 2024

View reviewed changes

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/PostgresIntegrationSuite.scala Outdated Show resolved Hide resolved

stefanbuk-db reviewed Sep 2, 2024

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala Outdated Show resolved Hide resolved

Improving test cases

ff4b45b

IvanK-db requested a review from stefanbuk-db September 3, 2024 09:24

stefanbuk-db reviewed Sep 3, 2024

View reviewed changes

Add filter tests

4b0865a

IvanK-db requested a review from stefanbuk-db September 4, 2024 10:01

beliefer reviewed Sep 4, 2024

View reviewed changes

IvanK-db requested a review from beliefer September 5, 2024 15:20

IvanK-db added 3 commits September 10, 2024 21:01

Merge branch 'apache:master' into SPARK-49162

e12cd61

H2 support push down date_trunc. Small refactor.

2414421

Merge branch 'SPARK-49162' of https://github.com/IvanK-db/spark into …

d06df08

…SPARK-49162

beliefer reviewed Sep 11, 2024

View reviewed changes

Improve tests

361b630

IvanK-db requested a review from beliefer September 12, 2024 09:08

beliefer reviewed Sep 12, 2024

View reviewed changes

Remove aggregate tests. Reuse checkFilterPushed

8626714

IvanK-db requested a review from beliefer September 12, 2024 15:07

beliefer approved these changes Sep 18, 2024

View reviewed changes

stefanbuk-db approved these changes Sep 18, 2024

View reviewed changes

milastdbx suggested changes Sep 18, 2024

View reviewed changes

cloud-fan reviewed Sep 18, 2024

View reviewed changes

Reverting merge commit. Adding test cases.

2d312a6

IvanK-db force-pushed the SPARK-49162 branch from c81f7df to 2d312a6 Compare September 20, 2024 11:24

IvanK-db added 11 commits September 20, 2024 13:25

Push down date_trunc function

ee09d83

Import Filter

24f9df1

Improving test cases

34e204e

Add filter tests

e2146df

H2 support push down date_trunc. Small refactor.

e8dae1c

Improve tests

ff1556d

Remove aggregate tests. Reuse checkFilterPushed

e85ccb8

Reverting merge commit. Adding test cases.

a93e27a

Merge branch 'SPARK-49162' of https://github.com/IvanK-db/spark into …

b2bad6d

…SPARK-49162

Fix expected filters.

cd58b51

Fix tests.

47367f7

beliefer reviewed Sep 23, 2024

View reviewed changes

IvanK-db added 2 commits September 26, 2024 19:17

Adding more test cases.

826d2ae

Removing unsupported test

afd5b23

IvanK-db requested review from beliefer, cloud-fan and milastdbx September 30, 2024 12:52

beliefer approved these changes Oct 4, 2024

View reviewed changes

github-actions bot added the Stale label Jan 17, 2025

github-actions bot closed this Jan 18, 2025

[SPARK-49162][SQL] Push down date_trunc function #47666

[SPARK-49162][SQL] Push down date_trunc function #47666

Uh oh!

Conversation

IvanK-db commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HyukjinKwon commented Sep 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanK-db Sep 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanK-db commented Sep 16, 2024

Uh oh!

beliefer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milastdbx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IvanK-db commented Aug 8, 2024 •

edited

Loading

IvanK-db Sep 5, 2024 •

edited

Loading

cloud-fan commented Oct 8, 2024 •

edited

Loading