Skip to content

[SPARK-26745][SQL] Revert count optimization in JSON datasource by SPARK-24959 - #23667

Closed
HyukjinKwon wants to merge 1 commit into
apache:masterfrom
HyukjinKwon:revert-SPARK-24959
Closed

[SPARK-26745][SQL] Revert count optimization in JSON datasource by SPARK-24959#23667
HyukjinKwon wants to merge 1 commit into
apache:masterfrom
HyukjinKwon:revert-SPARK-24959

Conversation

@HyukjinKwon

@HyukjinKwonHyukjinKwon commented Jan 28, 2019

Copy link
Copy Markdown
Member

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:

[{...}, {...}]

[]

{...}

# 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.

@HyukjinKwonHyukjinKwon mentioned this pull request Jan 28, 2019
Closed
@SparkQA

SparkQA commented Jan 28, 2019

Copy link
Copy Markdown

Test build #101745 has finished for PR 23667 at commit dd5b177.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

HyukjinKwon commented Jan 28, 2019

Copy link
Copy Markdown
MemberAuthor

retest this please

@SparkQA

SparkQA commented Jan 28, 2019

Copy link
Copy Markdown

Test build #101753 has finished for PR 23667 at commit dd5b177.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sumitsusumitsu mentioned this pull request Jan 28, 2019
Closed
srowen
srowen approved these changes Jan 29, 2019
View reviewed changes

@srowensrowen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a straight revert right? Looks OK. I agree we need to undo it for now because of the correctness issue. And then merge #23602

@HyukjinKwon

HyukjinKwon commented Jan 30, 2019

Copy link
Copy Markdown
MemberAuthor

Will get this in in few days if there's no objection.

@SparkQA

SparkQA commented Jan 31, 2019

Copy link
Copy Markdown

Test build #101926 has finished for PR 23667 at commit 466e3dd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon

HyukjinKwon commented Jan 31, 2019

Copy link
Copy Markdown
MemberAuthor

Merged to master.

I am going to open a backport soon.

cloud-fan
cloud-fan reviewed Jan 31, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala
} else {
// If `columnPruning` enabled and partition attributes scanned only,
// `schema` gets empty.
(_: String) => InternalRow.empty

@cloud-fancloud-fanJan 31, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too long ago and I can't remember the details. Does it mean we still have this count optimization for CSV? does it work in multiline mode?

@HyukjinKwonHyukjinKwonJan 31, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does for CSV when multiline is off and, for miltiline mode it executes a different code path.

UnivocityParser.parseStream -> UnivocityParser.convert

asfgit pushed a commit that referenced this pull request Feb 6, 2019
Branden Smith@HyukjinKwon
[SPARK-26745][SQL][TESTS] JsonSuite test case: empty line -> 0 record…
63bced9
… 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>
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@HyukjinKwonJackey Lee
[SPARK-26745][SQL] Revert count optimization in JSON datasource by SP…
183068c
…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.
Closesapache#23667 from HyukjinKwon/revert-SPARK-24959.
Authored-by: Hyukjin Kwon <gurwls223@apache.org>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
Branden SmithJackey Lee
[SPARK-26745][SQL][TESTS] JsonSuite test case: empty line -> 0 record…
db32fc0
… 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.
Closesapache#23674 from sumitsu/json_emptyline_count_test.
Authored-by: Branden Smith <branden.smith@publicismedia.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
dongjoon-hyun pushed a commit that referenced this pull request Apr 27, 2019
Branden Smith@dongjoon-hyun
[SPARK-26745][SQL][TESTS] JsonSuite test case: empty line -> 0 record…
ba9e12d
… 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>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Jul 23, 2019
Branden Smith@kai-chi
[SPARK-26745][SQL][TESTS] JsonSuite test case: empty line -> 0 record…
792113b
… 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.
Closesapache#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>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Jul 25, 2019
Branden Smith@kai-chi
[SPARK-26745][SQL][TESTS] JsonSuite test case: empty line -> 0 record…
371ad36
… 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.
Closesapache#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>
kai-chi pushed a commit to kai-chi/spark that referenced this pull request Aug 1, 2019
Branden Smith@kai-chi
[SPARK-26745][SQL][TESTS] JsonSuite test case: empty line -> 0 record…
bb2dc4e
… 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.
Closesapache#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>
@HyukjinKwon
HyukjinKwon deleted the revert-SPARK-24959 branch March 3, 2020 01:20
@MaxGekk

MaxGekk commented Jul 7, 2020

Copy link
Copy Markdown
Member

Not only in JSON, in CSV as well.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan left review comments
@srowensrowensrowen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@HyukjinKwon@SparkQA@MaxGekk@srowen@cloud-fan