-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
Revert [SPARK-23094] Fix invalid character handling in JsonDataSource #20614
Conversation
LGTM, my initial assumption that files had to be UTF-8 encoded was a wrong one :( |
Wait .. do we formally support UTF-16 encoded files in line by line parsing via JSON? I think this accidentally works because the newline character in UTF-8 is the part of UTF-16. I think I am fine to revert it but how about we avoid adding a test? |
Test build #87456 has finished for PR 20614 at commit
|
The failure seems to be irrelevant to this.
|
Retest this please |
Saw the history. Our UTF-16 support is pretty weak. Let me revert the test case. |
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.
Seems we should remove the UTF-16 file too.
LGTM too.
Test build #87467 has finished for PR 20614 at commit
|
Since this is just to remove the file and the previous test already passed, I would merge it to master/2.3. Thanks! |
BTW, the ongoing test will be killed by -9 at midnight |
## What changes were proposed in this pull request? This PR is to revert the PR #20302, because it causes a regression. ## How was this patch tested? N/A Author: gatorsmile <gatorsmile@gmail.com> Closes #20614 from gatorsmile/revertJsonFix. (cherry picked from commit 95e4b49) Signed-off-by: gatorsmile <gatorsmile@gmail.com>
Test build #87470 has finished for PR 20614 at commit
|
Test build #87473 has finished for PR 20614 at commit
|
What changes were proposed in this pull request?
This PR is to revert the PR #20302, because it causes a regression.
How was this patch tested?
N/A