-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-39002][SQL] StringEndsWith/Contains support push down to Parquet #36328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
e1a9def
e97cd22
45e13b6
4ece85e
4c4e2c8
e1a263d
2a34968
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -81,7 +81,7 @@ abstract class ParquetFilterSuite extends QueryTest with ParquetTest with Shared | |||
| datetimeRebaseSpec: RebaseSpec = RebaseSpec(LegacyBehaviorPolicy.CORRECTED) | ||||
| ): ParquetFilters = | ||||
| new ParquetFilters(schema, conf.parquetFilterPushDownDate, conf.parquetFilterPushDownTimestamp, | ||||
| conf.parquetFilterPushDownDecimal, conf.parquetFilterPushDownStringStartWith, | ||||
| conf.parquetFilterPushDownDecimal, conf.parquetFilterPushDownStringPredicate, | ||||
| conf.parquetFilterPushDownInFilterThreshold, | ||||
| caseSensitive.getOrElse(conf.caseSensitiveAnalysis), | ||||
| datetimeRebaseSpec) | ||||
|
|
@@ -207,20 +207,24 @@ abstract class ParquetFilterSuite extends QueryTest with ParquetTest with Shared | |||
| } | ||||
| } | ||||
|
|
||||
| // This function tests that exactly go through the `canDrop` and `inverseCanDrop`. | ||||
| private def testStringStartsWith(dataFrame: DataFrame, filter: String): Unit = { | ||||
| // This function tests that exactly go through the `keep`, `canDrop` and `inverseCanDrop`. | ||||
| private def testStringPredicate(dataFrame: DataFrame, filter: String, | ||||
| shouldFilterOut: Boolean, enableDictionary: Boolean = true): Unit = { | ||||
| withTempPath { dir => | ||||
| val path = dir.getCanonicalPath | ||||
| dataFrame.write.option("parquet.block.size", 512).parquet(path) | ||||
| dataFrame.write | ||||
| .option("parquet.block.size", 512) | ||||
| .option(ParquetOutputFormat.ENABLE_DICTIONARY, enableDictionary) | ||||
| .parquet(path) | ||||
| Seq(true, false).foreach { pushDown => | ||||
| withSQLConf( | ||||
| SQLConf.PARQUET_FILTER_PUSHDOWN_STRING_STARTSWITH_ENABLED.key -> pushDown.toString) { | ||||
| SQLConf.PARQUET_FILTER_PUSHDOWN_STRING_PREDICATE_ENABLED.key -> pushDown.toString) { | ||||
| val accu = new NumRowGroupsAcc | ||||
| sparkContext.register(accu) | ||||
|
|
||||
| val df = spark.read.parquet(path).filter(filter) | ||||
| df.foreachPartition((it: Iterator[Row]) => it.foreach(v => accu.add(0))) | ||||
| if (pushDown) { | ||||
| if (pushDown && shouldFilterOut) { | ||||
| assert(accu.value == 0) | ||||
| } else { | ||||
| assert(accu.value > 0) | ||||
|
|
@@ -970,7 +974,12 @@ abstract class ParquetFilterSuite extends QueryTest with ParquetTest with Shared | |||
| )) | ||||
|
|
||||
| val parquetSchema = new SparkToParquetSchemaConverter(conf).convert(schema) | ||||
| val parquetFilters = createParquetFilters(parquetSchema) | ||||
| // Following tests are used to check one arm of AND/OR can't be pushed down, | ||||
| // so we disable string predicate pushdown here | ||||
| var parquetFilters: ParquetFilters = null | ||||
| withSQLConf(SQLConf.PARQUET_FILTER_PUSHDOWN_STRING_PREDICATE_ENABLED.key -> "false") { | ||||
| parquetFilters = createParquetFilters(parquetSchema) | ||||
| } | ||||
| assertResult(Some(and( | ||||
| lt(intColumn("a"), 10: Integer), | ||||
| gt(doubleColumn("c"), 1.5: java.lang.Double))) | ||||
|
|
@@ -1114,7 +1123,12 @@ abstract class ParquetFilterSuite extends QueryTest with ParquetTest with Shared | |||
| )) | ||||
|
|
||||
| val parquetSchema = new SparkToParquetSchemaConverter(conf).convert(schema) | ||||
| val parquetFilters = createParquetFilters(parquetSchema) | ||||
| // Following tests are used to check one arm of AND/OR can't be pushed down, | ||||
| // so we disable string predicate pushdown here | ||||
| var parquetFilters: ParquetFilters = null | ||||
| withSQLConf(SQLConf.PARQUET_FILTER_PUSHDOWN_STRING_PREDICATE_ENABLED.key -> "false") { | ||||
| parquetFilters = createParquetFilters(parquetSchema) | ||||
| } | ||||
| // Testing | ||||
| // case sources.Or(lhs, rhs) => | ||||
| // ... | ||||
|
|
@@ -1169,7 +1183,12 @@ abstract class ParquetFilterSuite extends QueryTest with ParquetTest with Shared | |||
| )) | ||||
|
|
||||
| val parquetSchema = new SparkToParquetSchemaConverter(conf).convert(schema) | ||||
| val parquetFilters = createParquetFilters(parquetSchema) | ||||
| // Following tests are used to check one arm of AND/OR can't be pushed down, | ||||
| // so we disable string predicate pushdown here | ||||
| var parquetFilters: ParquetFilters = null | ||||
| withSQLConf(SQLConf.PARQUET_FILTER_PUSHDOWN_STRING_PREDICATE_ENABLED.key -> "false") { | ||||
| parquetFilters = createParquetFilters(parquetSchema) | ||||
| } | ||||
| assertResult(Seq(sources.And(sources.LessThan("a", 10), sources.GreaterThan("c", 1.5D)))) { | ||||
| parquetFilters.convertibleFilters( | ||||
| Seq(sources.And( | ||||
|
|
@@ -1476,12 +1495,50 @@ abstract class ParquetFilterSuite extends QueryTest with ParquetTest with Shared | |||
| classOf[UserDefinedByInstance[_, _]], | ||||
| Seq.empty[Row]) | ||||
| } | ||||
| } | ||||
|
|
||||
| test("filter pushdown - StringPredicate") { | ||||
| import testImplicits._ | ||||
| // Test canDrop() has taken effect | ||||
| testStringStartsWith(spark.range(1024).map(_.toString).toDF(), "value like 'a%'") | ||||
| // Test inverseCanDrop() has taken effect | ||||
| testStringStartsWith(spark.range(1024).map(c => "100").toDF(), "value not like '10%'") | ||||
| // keep() should take effect on StartsWith/EndsWith/Contains | ||||
| Seq( | ||||
| "value like 'a%'", // StartsWith | ||||
| "value like '%a'", // EndsWith | ||||
| "value like '%a%'" // Contains | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A quick comment. How does this verify the "keep()" test? Shouldn't it also be "canDrop()"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this test assume that dictionary filtering is enabled or not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the test is buggy and does not reflect the actual implementation of the filter. NumRowGroupAcc does not actually count row groups, it counts the number of records passed through the filter. For example, for the contains filter we should still read all of the row groups. Example of the log: Can the author update the test to reflect the implementation? cc @cloud-fan @sunchao.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @sadikovi , the NumRowGroupsAcc is the actually filtered row groups, you can find it here Line 130 in a1aa200
As to the When we test Correct me if I'm wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that was my point - the test needs to be updated to make sure dictionary pages are written and the dictionary filtering is enabled. Without it, the test does not verify the implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you open a follow-up PR to update the test? You can explicitly enable dictionary filtering in the test for the "keep" part of the test to highlight that the test passes due to dictionary filtering, otherwise it could be confusing for people.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enabling dictionary does not control dictionary filtering, there is a separate flag for it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the discussions here, seems this is not a simple thing. @sadikovi can you open a followup PR directly to demonstrate your idea? |
||||
| ).foreach { filter => | ||||
| testStringPredicate( | ||||
| // dictionary will be generated since there are duplicated values | ||||
| spark.range(1000).map(t => (t % 10).toString).toDF(), | ||||
| filter, | ||||
| true) | ||||
| } | ||||
|
|
||||
| // canDrop() should take effect on StartsWith, | ||||
| // and has no effect on EndsWith/Contains | ||||
| Seq( | ||||
| ("value like 'a%'", true), // StartsWith | ||||
| ("value like '%a'", false), // EndsWith | ||||
| ("value like '%a%'", false) // Contains | ||||
| ).foreach { case (filter, shouldFilterOut) => | ||||
| testStringPredicate( | ||||
| spark.range(1024).map(_.toString).toDF(), | ||||
| filter, | ||||
| shouldFilterOut, | ||||
| enableDictionary = false) | ||||
| } | ||||
|
|
||||
| // inverseCanDrop() should take effect on StartsWith, | ||||
| // and has no effect on EndsWith/Contains | ||||
| Seq( | ||||
| ("value not like '10%'", true), // StartsWith | ||||
| ("value not like '%10'", false), // EndsWith | ||||
| ("value not like '%10%'", false) // Contains | ||||
| ).foreach { case (filter, shouldFilterOut) => | ||||
| testStringPredicate( | ||||
| spark.range(1024).map(c => "100").toDF(), | ||||
| filter, | ||||
| shouldFilterOut, | ||||
| enableDictionary = false) | ||||
| } | ||||
| } | ||||
|
|
||||
| test("SPARK-17091: Convert IN predicate to Parquet filter push-down") { | ||||
|
|
||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
spark.sql.parquet.filterPushdown.string.startsWithis internal why not replacing it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid exising users who have already use it.