[SPARK-26930][SQL] Tests in ParquetFilterSuite don't verify filter class#23855
[SPARK-26930][SQL] Tests in ParquetFilterSuite don't verify filter class#23855nandorKollar wants to merge 2 commits intoapache:masterfrom
Conversation
|
@HyukjinKwon would you mind have a look at this PR and share your thought? The solution is simple (though suboptimal, since it doesn't check the entire filter tree, but that would require a lot more modification in the test code), yet provides more value than before. |
...e/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFilterSuite.scala
Outdated
Show resolved
Hide resolved
|
@cloud-fan, can you take a quick look before getting this in? |
| new SparkToParquetSchemaConverter(conf).convert(df.schema), pred) | ||
| assert(maybeFilter.isDefined, s"Couldn't generate filter predicate for $pred") | ||
| // Doesn't bother checking type parameters here (e.g. `Eq[Integer]`) | ||
| maybeFilter.exists(_.getClass === filterClass) |
There was a problem hiding this comment.
@cloud-fan, to cut it short, ef77003 missed to add an assert. Pushed filters are now like
before: a == 1
after: is not null && a == 1
These are turned into Parquet filters and here, it checks the filter's class. I thought it's overkill to traverse tree and/or whitelisting somehow with more codes. Currently, it simply checks the root filter after disabling the constraint filter.
|
ok to test |
|
LGTM |
|
ok to test |
1 similar comment
|
ok to test |
|
Test build #102608 has finished for PR 23855 at commit
|
|
Merged to master. |
|
Oh, it was you who reviewed my Parquet PR! :D. |
|
Thanks @HyukjinKwon and @cloud-fan for the review! @HyukjinKwon not impossible that I reviewed your PR on Parquet, I'm active there, occasionally do reviews. :) |
5 participants
What changes were proposed in this pull request?
Add assert to verify predicate class in ParquetFilterSuite
How was this patch tested?
Ran ParquetFilterSuite, tests passed