[SPARK-26745][SQL] Skip empty lines in JSON-derived DataFrames when skipParsing optimization in effect #23665
Conversation
…into json_emptyline_count
|
cc: @HyukjinKwon @MaxGekk |
|
ok to test |
| } | ||
| } else { | ||
| rawParser.apply(input).toIterator.map(row => toResultRow(Some(row), () => null)) | ||
| } |
There was a problem hiding this comment.
We need to modify this file? Since this is a issue in json stuffs, I think its better to handle this case in the json parser side. Can't we do handle this in the same way with CSV one?, e.g.,
There was a problem hiding this comment.
One of my earlier revisions worked in the way you suggest (if I've understood your point correctly); I changed it in order to avoid redundant empty-line filtering in the case where the full parser has to run anyway (i.e. where skipParsing == false).
What do you think? Is that a valid optimization, or is it better to do it on the JSON side as in 13942b8 to avoid changes to FailureSafeParser?
There was a problem hiding this comment.
I thought this: maropu@f4df907
But, you should follow other guys who are familiar this part, @HyukjinKwon and @MaxGekk
| val withEmptyLineData = Array(Map("a" -> 1, "b" -> 2, "c" -> 3), | ||
| Map("a" -> 4, "b" -> 5, "c" -> 6), | ||
| Map("a" -> 7, "b" -> 8, "c" -> 9)) | ||
| val df = spark.read.json("src/test/resources/test-data/with-empty-line.json") |
| assert(df.count() === withEmptyLineData.length, | ||
| "JSON DataFrame parsed-count should exclude whitespace-only lines") | ||
| val collected = df.collect().map(_.getValuesMap(Seq("a", "b", "c"))) | ||
| assert(collected === withEmptyLineData) |
| assert(df.count() === withEmptyLineData.length, | ||
| "JSON DataFrame unparsed-count should exclude whitespace-only lines") | ||
| // cache and collect to check that count stays stable under those operations | ||
| df.cache() |
| rawParser.apply(input).toIterator.map(row => toResultRow(Some(row), () => null)) | ||
| } | ||
| if (skipParsing) { | ||
| if (unparsedRecordIsNonEmpty(input)) { |
There was a problem hiding this comment.
I think we should rather revert #21909. I think #21909 was a bandaid fix and this is another bandaid fix for that.
JacksonParser itself can produce no record or multiple records. Previous code path assumed that it always produce a single record, and the current fix it checked the input again outside of JacksonParser.
There is another problem from #21909 . It also looks going to produce incorrect counts when the input json is an array:
$ cat tmp.json
[{"a": 1}, {"a": 2}]Current master:
scala> spark.read.json("tmp.json").show()
+---+
| a|
+---+
| 1|
| 2|
+---+
scala> spark.read.json("tmp.json").count()
res1: Long = 1Spark 2.3.1:
scala> spark.read.json("tmp.json").show()
+---+
| a|
+---+
| 1|
| 2|
+---+
scala> spark.read.json("tmp.json").count()
res1: Long = 2There was a problem hiding this comment.
cc @cloud-fan and @gatorsmile, if you don't mind, I would like to revert #21909. WDYT?
There was a problem hiding this comment.
that's a good catch! I think the idea of count optimization still makes sense, but our parser is so flexible and there are many corner cases we need to think of.
cc @MaxGekk , how hard do you think it is to fix it? If it's too hard, +1 to revert it. I think a safer approach maybe, only enable this count optimization under some certain cases that are 100% safe. (whitelist).
There was a problem hiding this comment.
Yea, that's safer approach if possible but to do that, we should manually check the input like the current PR. It doesn't looks a good idea to me to check input outside of JacksonParser.
The problem is, we cannot distinguish the cases below without parsing:
[{...}, {...}]
[]
{...}
# empty string
One line (input: IN) can be, 0 record, 1 record and multiple records.
There was a problem hiding this comment.
The case when an user sets StructType for arrays, can be excluded from the count optimization in advance.
Regarding empty (blank) string, before #23543 they are considered as bad records (appear in results). And count() produced pretty consistent results.
As far as you know, in the case of count we have empty StructType as the required schema. It means we don't have to fully parse the input and convert all field to desired types. It means count() can "count" bad records. And we cannot compare number of rows in show() output to count(). There are always the case when the number of rows can be different. I think we should answer to more generic question - which input conform to empty StructType(). After that it should be clear what kind of optimization can be applied for count(). Possible answers:
- readable string by Hadoop LineReader
- any text with length > 0 ( we can do cheap filtering
linesReader.filter(_.getLength > 0) - any valid UTF8 String (Hadoop's LineReader does not check that)
- anything parsable by Jackson parser
- Anything on which
FailureSafeParserdoesn't produce bad records (inPERMISSIVEmode). - String contains opened
{and closed}. And anything in between. - Valid JSON record. Parsable by Jackson in our case.
- Valid JSON + correct field values (can be converted to specified types).
- something else
Till we answer to the above question, reverting of the #21909 just move us from one "bad" behavior to one another "bad" behavior.
There was a problem hiding this comment.
[{...}, {...}] => 2
[] => 0
{...} => 1
# empty string => 0
I think the key here is, one line can produce 0 or 1 or more records, how to speed it up when we only care about counts? It looks to me that we can enable the count optimization only for {...}, and fallback to parsing for other cases. @MaxGekk do you think this is applicable? If it's a simple fix, let's do it for branch 2.4 as well, otherwise +1 for reverting it from 2.4 and re-do it at master.
There was a problem hiding this comment.
Like, how are we going to explain this to users?
Outside of datasources, count() has very well defined semantic - number of rows matched to required schema. In the case of count(), the required schema is Struct(Array()). From user's view, count() doesn't require any field presented in counted row.
If you would like to see the same number of rows in show() output and what count() returns, you need to push full datasource schema as required schema otherwise there are always malformed input on which you will see different results.
There was a problem hiding this comment.
otherwise there are always malformed input on which you will see different results
I think for permissive mode, the results(at least the counts) are always same even if some input are malformed? Otherwise, it seems like users only want to count the number of lines, and they should read the json files as text and do count.
There was a problem hiding this comment.
re: #23665 (comment)
This is reasoning about the count. (see below, continues)
I think for permissive mode, the results(at least the counts) are always same even if some input are malformed?
To be 100% about the correct results, we should always parse everything although we're doing the current way for optimization and it started to have some inconsistent results.
Yes, so we don't convert 100%. In that case, we should at least parse StructType() which I guess empty object {...}. I think that's what JSON did before the pointed PRs above.
Otherwise, it seems like users only want to count the number of lines, and they should read the json files as text and do count.
Yes, I agree. It shouldn't behaves like text source + count(). Let's revert anyway. I don't think this behaviour is ideal anyway.
For other behaviours, I was thinking about making a README.md that whitelists behaviours for both CSV and JSON for Spark developers under somewhere related JSON and CSV directory. It's a bit grunting job but sounds like it should be done. I could do this together @MaxGekk since he has worked on this area a lot as well.
There was a problem hiding this comment.
It looks to me that we can enable the count optimization only for {...}, and fallback to parsing for other cases. @MaxGekk do you think this is applicable? If it's a simple fix, let's do it for branch 2.4 as well, otherwise +1 for reverting it from 2.4 and re-do it at master.
We can but we should add a if-else for each input + checking some characters since it targets to avoid to parse.
|
Test build #101739 has finished for PR 23665 at commit
|
|
Thanks @HyukjinKwon ; I've created a new PR (#23674) which includes only the test components of this PR, and incorporates @maropu 's comments above regarding code style on that test case. |
|
Closing this since that's reverted. |
… in JSON datasource by ## What changes were proposed in this pull request? This PR reverts JSON count optimization part of #21909. We cannot distinguish the cases below without parsing: ``` [{...}, {...}] ``` ``` [] ``` ``` {...} ``` ```bash # empty string ``` when we `count()`. One line (input: IN) can be, 0 record, 1 record and multiple records and this is dependent on each input. See also #23665 (comment). ## How was this patch tested? Manually tested. Closes #23708 from HyukjinKwon/SPARK-26745-backport. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
… count ## What changes were proposed in this pull request? This PR consists of the `test` components of #23665 only, minus the associated patch from that PR. It adds a new unit test to `JsonSuite` which verifies that the `count()` returned from a `DataFrame` loaded from JSON containing empty lines does not include those empty lines in the record count. The test runs `count` prior to otherwise reading data from the `DataFrame`, so as to catch future cases where a pre-parsing optimization might result in `count` results inconsistent with existing behavior. This PR is intended to be deployed alongside #23667; `master` currently causes the test to fail, as described in [SPARK-26745](https://issues.apache.org/jira/browse/SPARK-26745). ## How was this patch tested? Manual testing, existing `JsonSuite` unit tests. Closes #23674 from sumitsu/json_emptyline_count_test. Authored-by: Branden Smith <branden.smith@publicismedia.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
…ARK-24959 ## What changes were proposed in this pull request? This PR reverts JSON count optimization part of apache#21909. We cannot distinguish the cases below without parsing: ``` [{...}, {...}] ``` ``` [] ``` ``` {...} ``` ```bash # empty string ``` when we `count()`. One line (input: IN) can be, 0 record, 1 record and multiple records and this is dependent on each input. See also apache#23665 (comment). ## How was this patch tested? Manually tested. Closes apache#23667 from HyukjinKwon/revert-SPARK-24959. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
… count ## What changes were proposed in this pull request? This PR consists of the `test` components of apache#23665 only, minus the associated patch from that PR. It adds a new unit test to `JsonSuite` which verifies that the `count()` returned from a `DataFrame` loaded from JSON containing empty lines does not include those empty lines in the record count. The test runs `count` prior to otherwise reading data from the `DataFrame`, so as to catch future cases where a pre-parsing optimization might result in `count` results inconsistent with existing behavior. This PR is intended to be deployed alongside apache#23667; `master` currently causes the test to fail, as described in [SPARK-26745](https://issues.apache.org/jira/browse/SPARK-26745). ## How was this patch tested? Manual testing, existing `JsonSuite` unit tests. Closes apache#23674 from sumitsu/json_emptyline_count_test. Authored-by: Branden Smith <branden.smith@publicismedia.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
… count This PR consists of the `test` components of #23665 only, minus the associated patch from that PR. It adds a new unit test to `JsonSuite` which verifies that the `count()` returned from a `DataFrame` loaded from JSON containing empty lines does not include those empty lines in the record count. The test runs `count` prior to otherwise reading data from the `DataFrame`, so as to catch future cases where a pre-parsing optimization might result in `count` results inconsistent with existing behavior. This PR is intended to be deployed alongside #23667; `master` currently causes the test to fail, as described in [SPARK-26745](https://issues.apache.org/jira/browse/SPARK-26745). Manual testing, existing `JsonSuite` unit tests. Closes #23674 from sumitsu/json_emptyline_count_test. Authored-by: Branden Smith <branden.smith@publicismedia.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 63bced9) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
… in JSON datasource by ## What changes were proposed in this pull request? This PR reverts JSON count optimization part of apache#21909. We cannot distinguish the cases below without parsing: ``` [{...}, {...}] ``` ``` [] ``` ``` {...} ``` ```bash # empty string ``` when we `count()`. One line (input: IN) can be, 0 record, 1 record and multiple records and this is dependent on each input. See also apache#23665 (comment). ## How was this patch tested? Manually tested. Closes apache#23708 from HyukjinKwon/SPARK-26745-backport. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
… count This PR consists of the `test` components of apache#23665 only, minus the associated patch from that PR. It adds a new unit test to `JsonSuite` which verifies that the `count()` returned from a `DataFrame` loaded from JSON containing empty lines does not include those empty lines in the record count. The test runs `count` prior to otherwise reading data from the `DataFrame`, so as to catch future cases where a pre-parsing optimization might result in `count` results inconsistent with existing behavior. This PR is intended to be deployed alongside apache#23667; `master` currently causes the test to fail, as described in [SPARK-26745](https://issues.apache.org/jira/browse/SPARK-26745). Manual testing, existing `JsonSuite` unit tests. Closes apache#23674 from sumitsu/json_emptyline_count_test. Authored-by: Branden Smith <branden.smith@publicismedia.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 63bced9) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
… in JSON datasource by ## What changes were proposed in this pull request? This PR reverts JSON count optimization part of apache#21909. We cannot distinguish the cases below without parsing: ``` [{...}, {...}] ``` ``` [] ``` ``` {...} ``` ```bash # empty string ``` when we `count()`. One line (input: IN) can be, 0 record, 1 record and multiple records and this is dependent on each input. See also apache#23665 (comment). ## How was this patch tested? Manually tested. Closes apache#23708 from HyukjinKwon/SPARK-26745-backport. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
… count This PR consists of the `test` components of apache#23665 only, minus the associated patch from that PR. It adds a new unit test to `JsonSuite` which verifies that the `count()` returned from a `DataFrame` loaded from JSON containing empty lines does not include those empty lines in the record count. The test runs `count` prior to otherwise reading data from the `DataFrame`, so as to catch future cases where a pre-parsing optimization might result in `count` results inconsistent with existing behavior. This PR is intended to be deployed alongside apache#23667; `master` currently causes the test to fail, as described in [SPARK-26745](https://issues.apache.org/jira/browse/SPARK-26745). Manual testing, existing `JsonSuite` unit tests. Closes apache#23674 from sumitsu/json_emptyline_count_test. Authored-by: Branden Smith <branden.smith@publicismedia.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 63bced9) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
… in JSON datasource by ## What changes were proposed in this pull request? This PR reverts JSON count optimization part of apache#21909. We cannot distinguish the cases below without parsing: ``` [{...}, {...}] ``` ``` [] ``` ``` {...} ``` ```bash # empty string ``` when we `count()`. One line (input: IN) can be, 0 record, 1 record and multiple records and this is dependent on each input. See also apache#23665 (comment). ## How was this patch tested? Manually tested. Closes apache#23708 from HyukjinKwon/SPARK-26745-backport. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
… count This PR consists of the `test` components of apache#23665 only, minus the associated patch from that PR. It adds a new unit test to `JsonSuite` which verifies that the `count()` returned from a `DataFrame` loaded from JSON containing empty lines does not include those empty lines in the record count. The test runs `count` prior to otherwise reading data from the `DataFrame`, so as to catch future cases where a pre-parsing optimization might result in `count` results inconsistent with existing behavior. This PR is intended to be deployed alongside apache#23667; `master` currently causes the test to fail, as described in [SPARK-26745](https://issues.apache.org/jira/browse/SPARK-26745). Manual testing, existing `JsonSuite` unit tests. Closes apache#23674 from sumitsu/json_emptyline_count_test. Authored-by: Branden Smith <branden.smith@publicismedia.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 63bced9) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
… in JSON datasource by
## What changes were proposed in this pull request?
This PR reverts JSON count optimization part of #21909.
We cannot distinguish the cases below without parsing:
```
[{...}, {...}]
```
```
[]
```
```
{...}
```
```bash
# empty string
```
when we `count()`. One line (input: IN) can be, 0 record, 1 record and multiple records and this is dependent on each input.
See also apache/spark#23665 (comment).
## How was this patch tested?
Manually tested.
Closes #23708 from HyukjinKwon/SPARK-26745-backport.
Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
(cherry picked from commit 2a83431)
What changes were proposed in this pull request?
This PR updates
FailureSafeParserto allow text-input data sources to optionally specify a "fast" emptiness check for records, to be applied in cases where full parsing is disabled (i.e. whereskipParsing==true: non-multiline + permissive-mode + empty schema).TextInputJsonDataSourceis updated such that it createsFailureSafeParserwith an emptiness check which filters out blank (or all-whitespace) lines. This behavior resolves SPARK-26745 by preventingcount()from including blank lines (which the full parser ignores) under conditions whereskipParsingis enabled.How was this patch tested?
Existing
JsonSuiteunit tests, supplemented by a new test case which verifies that pre-parsing and post-parsingcount()values are equal for JSON-derived DataFrames.JsonBenchmarkperformance test resultsThe
JsonBenchmarkperformance suite was executed on the following branches:sumitsu/spark:json_emptyline_count)apache/spark:masterbranchapache/spark:masterbranch, modified to not use the SPARK-24959 optimizationThe no-optimization code base was simulated by hard-coding the
skipParsingflag inFailureSafeParsertofalsefor the test.Compared with the no-optimization scenario, this PR appears to preserve most of the SPARK-24959 optimization performance gains, but there is a small performance regression compared with
master.Summary charts:
test environment:
with changes in this PR (branch:
sumitsu/spark:json_emptyline_count)apache/spark:masterbranchoptimization disabled