[SPARK-23849][SQL] Tests for the samplingRatio option of JSON datasource #20963
Conversation
|
Test build #88828 has finished for PR 20963 at commit
|
| } | ||
| writer.close() | ||
|
|
||
| val ds = spark.read.option("samplingRatio", 0.1).json(path.getCanonicalPath) |
There was a problem hiding this comment.
@MaxGekk, wouldn't this test be flaky?
There was a problem hiding this comment.
It could be if the partitionIndex is flaky here:
0
There was a problem hiding this comment.
OK but I don't think we are guaranteed to have always one partition here though. Shall we at least explicitly set spark.sql.files.maxPartitionBytes big enough with some comments?
I think we shouldn't encourage this way because it should likely be easy to be broken IMHO. I am fine with it anyway as I can't think of a better way on the other hand.
There was a problem hiding this comment.
It seems specifying only spark.sql.files.maxPartitionBytes is not enough. Please, look at the formula and slicing input files:
val maxSplitBytes = Math.min(defaultMaxSplitBytes, Math.max(openCostInBytes, bytesPerCore))
Is ok if I just check that file size is less than maxSplitBytes?
There was a problem hiding this comment.
yup, please set the appropriate numbers. I think it's fine if it has some comments so that we read and fix the tests if it's broken.
There was a problem hiding this comment.
^ It's based upon actual experience before. There was a similar case that the test was broken due to the number of partitions and it took me a while to debug it, https://issues.apache.org/jira/browse/SPARK-13728
|
|
||
| test("SPARK-23849: schema inferring touches less data if samplingRation < 1.0") { | ||
| val predefinedSample = Set[Int](2, 8, 15, 27, 30, 34, 35, 37, 44, 46, | ||
| 57, 62, 68, 72) |
There was a problem hiding this comment.
Not need to have so many elements in this set. Please combine the tests in your CSV PR.
Instead of calling json(), we can do it using format("json"). Then, you can combine the test cases for both CSV and Json.
|
LGTM. Thanks! Merged to master/ |
What changes were proposed in this pull request?
Proposed tests checks that only subset of input dataset is touched during schema inferring.