Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/8956#discussion_r42176719
--- Diff:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFilters.scala
---
@@ -48,6 +48,34 @@ private[sql] object ParquetFilters {
override def inverseCanDrop(statistics: Statistics[T]): Boolean = false
}
+ object StringFilter extends Enumeration {
+ type Mode = Value
+ val STARTS_WITH, ENDS_WITH, CONTAINS = Value
+ }
+
+ case class StringFilter(
+ v: java.lang.String,
+ mode: StringFilter.Mode) extends UserDefinedPredicate[Binary] with
Serializable {
+
+ private val compare = mode match {
+ case StringFilter.STARTS_WITH =>
+ (x: java.lang.String) => x.startsWith(v)
+ case StringFilter.ENDS_WITH =>
+ (x: java.lang.String) => x.endsWith(v)
+ case StringFilter.CONTAINS =>
+ (x: java.lang.String) => x.contains(v)
+ }
+
+ override def keep(value: Binary): Boolean = {
+ val str = value.toStringUsingUTF8()
+ compare(str)
+ }
--- End diff --
Instead of using a enum to dispatch the comparator function, I'd prefer to
use simple inheritance here, namely:
```scala
abstract class StringFilter extends UserDefinedPredicate[Binary] {
override def canDrop(statistics: Statistics[Binary]): Boolean = false
override def inverseCanDrop(statistics: Statistics[Binary]): Boolean =
false
}
case class StringStartsWithFilter(prefix: String) extends StringFilter {
override def keep(value: Binary): Boolean =
value.toStringUsingUTF8.startsWith(value)
}
case class StringEndsWithFilter(prefix: String) extends StringFilter {
override def keep(value: Binary): Boolean =
value.toStringUsingUTF8.endsWith(value)
}
case class StringContainsFilter(prefix: String) extends StringFilter {
override def keep(value: Binary): Boolean =
value.toStringUsingUTF8.contains(value)
}
```
The reasons are:
1. `keep` is on a performance critical code path, an extra level
indirection hurts performance.
1. `StringStartsWithFilter.canDrop` can leverage statistics information:
```scala
override def canDrop(statistics: Statistics[Binary]): Boolean = {
val max = statistics.getMax.toStringUsingUTF8
val min = statistics.getMin.toStringUsingUTF8
max < prefix || !min.startsWith(prefix) && min > prefix
}
```
It would be easier to override `canDrop` in the above version.
*However*, we should NOT add the above `canDrop` for now, because
parquet-mr 1.7.0 suffers from [PARQUET-251] [1], which may cause wrong query
results because of corrupted binary statistics information. We may add it
after upgrading parquet-mr to 1.8+ (hopefully 1.8.2 since we observed slight
performance regression in 1.8.1).
[1]: https://issues.apache.org/jira/browse/PARQUET-251
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
