Skip to content

Inconsistent result for DROPMALFORMED with pruned scan and unnecessarily casting all values when no fields are required #220

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.

Sign up for GitHub

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

Jump to bottom
Closed
HyukjinKwon wants to merge 2 commits into from
Closed

Inconsistent result for DROPMALFORMED with pruned scan and unnecessarily casting all values when no fields are required #220

HyukjinKwon wants to merge 2 commits into from

Conversation

@HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Dec 28, 2015

https://github.com/databricks/spark-csv/issues/218 https://github.com/databricks/spark-csv/issues/219

In this PR, I made the pruned scan try to parse all the values in columns when DROPMALFORMED is enabled and return only required fields.

In addition, I changed the condition for table scan. If required columns are empty, then it just produces empty rows.

@codecov-io
Copy link

codecov-io commented Dec 28, 2015

Current coverage is 85.82%

Merging #220 into master will increase coverage by +0.11% as of 5b06cea

@@            master    #220   diff @@
======================================
  Files           11      11       
  Stmts          497     501     +4
  Branches       146     148     +2
  Methods          0       0       
======================================
+ Hit            426     430     +4
  Partial          0       0       
  Missed          71      71

Review entire Coverage Diff as of 5b06cea

Powered by Codecov. Updated on successful CI builds.

@falaki
Copy link
Member

falaki commented Dec 28, 2015

Thanks @HyukjinKwon for investigating this. Do you think we should also include FAILFAST mode?

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Dec 28, 2015

@falaki Ah..if you meant handling some logics to work FAILFAST mode properly, actually it looks fine with FAILFAST mode as it just compares schema to tokens (which are always read all regardless of pruned scan or table scan).

falaki
falaki reviewed Dec 28, 2015
View reviewed changes
src/main/scala/com/databricks/spark/csv/CsvRelation.scala
Copy link
Member

@falaki falaki Dec 28, 2015

Choose a reason for hiding this comment

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

Why not simply using schemaFields?

Copy link
Member Author

@HyukjinKwon HyukjinKwon Dec 28, 2015

Choose a reason for hiding this comment

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

Because of the order of columns. The required columns can be in a different order with the original schema.
If you look through the code below, you will notice I slice the row by take().

@HyukjinKwon
Copy link
Member Author

HyukjinKwon commented Dec 28, 2015

And.. would you mind if you look through the codes, in particular, the variable names (although it is trivial)?
I am not too sure if the naming looks appropriate.

@falaki
Copy link
Member

falaki commented Dec 28, 2015

Thanks this looks good to me.

@falaki falaki closed this in 4a5ee81 Dec 28, 2015
@HyukjinKwon HyukjinKwon mentioned this pull request Feb 12, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@HyukjinKwon @codecov-io @falaki