dongjoon-hyun commented on code in PR #59641:
URL: https://github.com/apache/spark/pull/59641#discussion_r4326710138
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/types/ops/TimestampNanosParquetOps.scala:
##########
@@ -320,6 +320,16 @@ private[ops] object TimestampNanosParquetOps {
private def epochNanosInRange(encode: => JLong): Boolean =
try { encode; true } catch { case _: ArithmeticException => false }
+ // Below nanosecond precision, the reader truncates the stored
epoch-nanoseconds to the read
+ // precision (see nanosConverter and TimestampNanosVectorUpdater), while a
pushed down predicate
+ // compares the stored epoch-nanoseconds. E.g. a stored 00:00:00.123456789
is read as
+ // 00:00:00.1234567 for TIMESTAMP_NTZ(7), so a pushed down `ts =
00:00:00.1234567` would skip it.
+ private def acceptsNanosReadType(readType: DataType): Boolean = readType
match {
Review Comment:
Thank you. As in #59606, I'd like to keep this PR minimal as a correctness
fix for `branch-4.3` too. `<` and `>=` are exact only when the bound is on the
read grid, and `>` and `!=` become under-inclusive under `NOT` because
parquet-mr inverts `gt(v)` to `ltEq(v)` and `notEq(v)` to `eq(v)`. The default
precision of the nanosecond timestamp types is 9, so this applies only to the
explicit precisions 7 and 8, like `TIME(0)` to `TIME(5)` for the default
`TIME(6)`. I'll recover the pushdown by rewriting the bounds as you suggested
in a new JIRA issue for both `TIME` and `TIMESTAMP(NANOS)`.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/types/ops/TimestampNanosParquetOps.scala:
##########
@@ -339,6 +349,8 @@ private[ops] object TimestampNanosParquetOps {
case _ => false
}
+ override def acceptsReadType(readType: DataType): Boolean =
acceptsNanosReadType(readType)
Review Comment:
Thank you. I agree that it's the same rule. Since the bound rewriting needs
the storage precision in the ops anyway, let me revisit this hook, including
its default, in that new JIRA issue.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFilterSuite.scala:
##########
@@ -2416,11 +2416,14 @@ abstract class ParquetFilterSuite extends ParquetTest
with SharedSparkSession {
withSQLConf(
SQLConf.TIMESTAMP_NANOS_TYPES_ENABLED.key -> "true",
SQLConf.PARQUET_FILTER_PUSHDOWN_INFILTERTHRESHOLD.key -> "1") {
+ // The columns are read with precision 9. A lower read precision
truncates the values, so
+ // only IsNull and IsNotNull are pushed down then.
Seq(7, 8, 9).foreach { p =>
Review Comment:
Thank you. Updated to drop the loop.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFilterSuite.scala:
##########
@@ -2557,6 +2561,86 @@ abstract class ParquetFilterSuite extends ParquetTest
with SharedSparkSession {
}
}
+ test("SPARK-60380: filter pushdown - nanosecond timestamp read with a lower
precision") {
Review Comment:
`ParquetFilterSuite` is an abstract class which runs as
`ParquetV1FilterSuite` and `ParquetV2FilterSuite`, and they set
`spark.sql.sources.useV1SourceList` to `parquet` and an empty string
respectively. So, this test case covers V2 too. Without this fix, it fails in
`ParquetV2FilterSuite` as well, with `RelationV2` in the plan.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFilterSuite.scala:
##########
@@ -2557,6 +2561,86 @@ abstract class ParquetFilterSuite extends ParquetTest
with SharedSparkSession {
}
}
+ test("SPARK-60380: filter pushdown - nanosecond timestamp read with a lower
precision") {
+ // A TIMESTAMP(9) value 1970-01-01 00:00:00.123456789 read as TIMESTAMP(7)
is truncated to
+ // 00:00:00.1234567, while Parquet evaluates a pushed down predicate
against the stored value.
Review Comment:
Thank you. Updated to loop over the read precisions 7 and 8.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
