[SPARK-31489][SPARK-31488][SQL] Translate date values of pushed down filters to `java.sql.Date` #28272

MaxGekk · 2020-04-20T09:16:13Z

What changes were proposed in this pull request?

Always translate date values of pushed down filters to java.sql.Date independently from the SQL config spark.sql.datetime.java8API.enabled.

Why are the changes needed?

For backward compatibility with existing implementations of datasources. For example, the following exception is thrown by ORC datasource when spark.sql.datetime.java8API.enabled is set to true:

Wrong value class java.time.LocalDate for DATE.EQUALS leaf
java.lang.IllegalArgumentException: Wrong value class java.time.LocalDate for DATE.EQUALS leaf
	at org.apache.hadoop.hive.ql.io.sarg.SearchArgumentImpl$PredicateLeafImpl.checkLiteralType(SearchArgumentImpl.java:192)
	at org.apache.hadoop.hive.ql.io.sarg.SearchArgumentImpl$PredicateLeafImpl.<init>(SearchArgumentImpl.java:75)
	at org.apache.hadoop.hive.ql.io.sarg.SearchArgumentImpl$BuilderImpl.equals(SearchArgumentImpl.java:352)
	at org.apache.spark.sql.execution.datasources.orc.OrcFilters$.buildLeafSearchArgument(OrcFilters.scala:229)

Before the changes, date filters are not pushed down to Parquet datasource when spark.sql.datetime.java8API.enabled is true.

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

Added a test to ParquetFilterSuite and to OrcFilterSuite.

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala

SparkQA · 2020-04-20T14:37:43Z

Test build #121516 has finished for PR 28272 at commit 3dca84d.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala

SparkQA · 2020-04-20T19:31:55Z

Test build #121537 has finished for PR 28272 at commit 9ce9a34.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2020-04-20T20:29:33Z

jenkins, retest this, please

This reverts commit d52fe37.

SparkQA · 2020-04-20T21:23:14Z

Test build #121538 has finished for PR 28272 at commit 32fb0ea.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-21T00:53:26Z

Test build #121543 has finished for PR 28272 at commit d52fe37.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-21T01:13:06Z

Test build #121544 has finished for PR 28272 at commit d52fe37.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-21T01:14:52Z

Test build #121545 has finished for PR 28272 at commit ff2ca3f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2020-04-21T06:42:03Z

jenkins, retest this, please

SparkQA · 2020-04-21T07:05:02Z

Test build #121569 has finished for PR 28272 at commit ff2ca3f.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2020-04-21T07:26:10Z

jenkins, retest this, please

cloud-fan · 2020-04-21T07:38:35Z

...e/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFilterSuite.scala

    }
  }
+
+  test("filter pushdown - local date") {


can we just update test("filter pushdown - date") to test with DATETIME_JAVA8API_ENABLED on and off, so that we have less duplicated code?

cloud-fan · 2020-04-21T07:38:55Z

...core/v1.2/src/test/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilterSuite.scala

    }
  }
+
+  test("filter pushdown - local date") {


cloud-fan · 2020-04-21T07:39:02Z

...core/v2.3/src/test/scala/org/apache/spark/sql/execution/datasources/orc/OrcFilterSuite.scala

    }
  }
+
+  test("filter pushdown - local date") {


HyukjinKwon · 2020-04-21T08:23:56Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala

  private def translateLeafNodeFilter(predicate: Expression): Option[Filter] = predicate match {
    case expressions.EqualTo(PushableColumn(name), Literal(v, t)) =>
-      Some(sources.EqualTo(name, convertToScala(v, t)))
+      Some(sources.EqualTo(name, convertToScala(v, t, false)))


I guess we're treating this as a temp fix for Spark 3.0?
Looks like ideally we should support Java 8 datetime instances for this interface as well when spark.sql.datetime.java8API.enabled is enabled. It could cause more confusion. In addition, seems like spark.sql.datetime.java8API.enabled is disabled by default, too.

I think it's problematic to let the java8 config also control the value type inside Filter, as it can break existing DS v1 implementations. It's a bit unfortunate that we don't document clearly what the value type can be for Filter, but if we do, it's not user-friendly to say "the value type depends on xxx config". This just makes it harder to implement data source filter pushdown.

@HyukjinKwon Taking into account #23811 (comment), the flag won't be enabled by default in the near future.

SparkQA · 2020-04-21T12:25:03Z

Test build #121574 has finished for PR 28272 at commit ff2ca3f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2020-04-21T19:48:06Z

jenkins, retest this, please

SparkQA · 2020-04-22T00:31:42Z

Test build #121594 has finished for PR 28272 at commit 2973dd7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-22T00:41:06Z

Test build #121593 has finished for PR 28272 at commit d7b2ece.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2020-04-22T05:46:15Z

@cloud-fan @HyukjinKwon @dongjoon-hyun Please, take a look at the PR if you have time.

…` values in ORC ### What changes were proposed in this pull request? Convert `java.time.LocalDate` to `java.sql.Date` in pushed down filters to ORC datasource when Java 8 time API enabled. Closes #28272 ### Why are the changes needed? The changes fix the exception raised while pushing date filters when `spark.sql.datetime.java8API.enabled` is set to `true`: ``` Wrong value class java.time.LocalDate for DATE.EQUALS leaf java.lang.IllegalArgumentException: Wrong value class java.time.LocalDate for DATE.EQUALS leaf at org.apache.hadoop.hive.ql.io.sarg.SearchArgumentImpl$PredicateLeafImpl.checkLiteralType(SearchArgumentImpl.java:192) at org.apache.hadoop.hive.ql.io.sarg.SearchArgumentImpl$PredicateLeafImpl.<init>(SearchArgumentImpl.java:75) at org.apache.hadoop.hive.ql.io.sarg.SearchArgumentImpl$BuilderImpl.equals(SearchArgumentImpl.java:352) at org.apache.spark.sql.execution.datasources.orc.OrcFilters$.buildLeafSearchArgument(OrcFilters.scala:229) ``` ### Does this PR introduce any user-facing change? Yes ### How was this patch tested? Added tests to `OrcFilterSuite`. Closes #28261 from MaxGekk/orc-date-filter-pushdown. Authored-by: Max Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit bd139bd) Signed-off-by: Dongjoon Hyun <[email protected]>

Translate filter values via translateFilterValues

3dca84d

probot-autolabeler bot added the SQL label Apr 20, 2020

MaxGekk mentioned this pull request Apr 20, 2020

[SPARK-31489][SQL] Fix pushing down filters with java.time.LocalDate values in ORC #28261

Closed

cloud-fan reviewed Apr 20, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala Outdated Show resolved Hide resolved

MaxGekk added 2 commits April 20, 2020 19:19

Add useJava8DateTimeApi param

9ce9a34

private[sql] -> private

32fb0ea

dongjoon-hyun reviewed Apr 20, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala Outdated Show resolved Hide resolved

Remove useJava8DateTimeApi from convertToScala()

d52fe37

MaxGekk changed the title ~~[SPARK-31489][SPARK-31488][SQL] Translate date values of pushed down filters to java.sql.Date~~ [SPARK-31489][SPARK-31488][SQL][test-hive1.2] Translate date values of pushed down filters to java.sql.Date Apr 20, 2020

MaxGekk added 2 commits April 20, 2020 23:41

Revert "Remove useJava8DateTimeApi from convertToScala()"

ac0b27a

This reverts commit d52fe37.

Remove default value in convertToScala

ff2ca3f

MaxGekk changed the title ~~[SPARK-31489][SPARK-31488][SQL][test-hive1.2] Translate date values of pushed down filters to java.sql.Date~~ [SPARK-31489][SPARK-31488][SQL] Translate date values of pushed down filters to java.sql.Date Apr 21, 2020

cloud-fan reviewed Apr 21, 2020

View reviewed changes

HyukjinKwon reviewed Apr 21, 2020

View reviewed changes

MaxGekk added 3 commits April 21, 2020 21:59

Dedup code in ParquetFilterSuite

2a82a10

Dedup code in OrcFilterSuite

d7b2ece

Dedup code in OrcFilterSuite v1.2

2973dd7

MaxGekk changed the title ~~[SPARK-31489][SPARK-31488][SQL] Translate date values of pushed down filters to java.sql.Date~~ [SPARK-31489][SPARK-31488][SQL][test-hive1.2] Translate date values of pushed down filters to java.sql.Date Apr 21, 2020

MaxGekk changed the title ~~[SPARK-31489][SPARK-31488][SQL][test-hive1.2] Translate date values of pushed down filters to java.sql.Date~~ [SPARK-31489][SPARK-31488][SQL] Translate date values of pushed down filters to java.sql.Date Apr 22, 2020

MaxGekk mentioned this pull request Apr 22, 2020

[MINOR][SQL] Add comments for filters values and return values of Row.get()/apply() #28300

Closed

dongjoon-hyun closed this in bd139bd Apr 26, 2020

MaxGekk deleted the fix-translateLeafNodeFilter branch June 5, 2020 19:48

[SPARK-31489][SPARK-31488][SQL] Translate date values of pushed down filters to java.sql.Date #28272

[SPARK-31489][SPARK-31488][SQL] Translate date values of pushed down filters to java.sql.Date #28272

Uh oh!

Conversation

MaxGekk commented Apr 20, 2020 • 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?

Uh oh!

Uh oh!

SparkQA commented Apr 20, 2020

Uh oh!

Uh oh!

SparkQA commented Apr 20, 2020

Uh oh!

MaxGekk commented Apr 20, 2020

Uh oh!

SparkQA commented Apr 20, 2020

Uh oh!

SparkQA commented Apr 21, 2020

Uh oh!

SparkQA commented Apr 21, 2020

Uh oh!

SparkQA commented Apr 21, 2020

Uh oh!

MaxGekk commented Apr 21, 2020

Uh oh!

SparkQA commented Apr 21, 2020

Uh oh!

MaxGekk commented Apr 21, 2020

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!

SparkQA commented Apr 21, 2020

Uh oh!

MaxGekk commented Apr 21, 2020

Uh oh!

SparkQA commented Apr 22, 2020

Uh oh!

SparkQA commented Apr 22, 2020

Uh oh!

MaxGekk commented Apr 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[SPARK-31489][SPARK-31488][SQL] Translate date values of pushed down filters to `java.sql.Date` #28272

[SPARK-31489][SPARK-31488][SQL] Translate date values of pushed down filters to `java.sql.Date` #28272

MaxGekk commented Apr 20, 2020 •

edited

Loading