search

databricks / spark-avro

Avro Data Source for Apache Spark
http://databricks.com/
Apache License 2.0
539 stars 310 forks source link

Fix null Timestamp/Date coercion to 0 epoch. #262

Closed aa8y closed 6 years ago

aa8y commented 6 years ago

The current implementation of Long to Date / Timestamp coercion does not take into account null values. Due to this when a null is cast into a Long, it ends up as 0 (as Scala Long does not support null). This PR is a fix for that.

codecov-io commented 6 years ago

Codecov Report

Merging #262 into master will increase coverage by 1.41%. The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage    90.8%   92.21%   +1.41%     
==========================================
  Files           5        5              
  Lines         337      321      -16     
  Branches       54       43      -11     
==========================================
- Hits          306      296      -10     
+ Misses         31       25       -6
aa8y commented 6 years ago

@gengliangwang Please let me know if you need anything from me to merge this PR.

aa8y commented 6 years ago

@JoshRosen and @marmbrus Anything I can do to help move this forward?

aa8y commented 6 years ago

@gengliangwang I've made the requested changes. Please let me know if you need anything else.

gengliangwang commented 6 years ago

There is a lot if (item == null)... in current code, can you remove the redundant code, and create a wrapper over createConverter for checking the null if schema is nullable?

I prefer to do it in this PR. If you don't have time, I can merge this one and do it later.

aa8y commented 6 years ago

@gengliangwang I've made some changes. Let me know if it looks like what you had in mind.

aa8y commented 6 years ago

@gengliangwang I removed the braces because they were inconsistent within the function. Some places had it and some places didn't. So I removed them from a few places for consistency. I think it reads better this way but if you insist I can add them back.

gengliangwang commented 6 years ago

@aa8y I see, you don't have to add them back.

aa8y commented 6 years ago

@gengliangwang Is this ready to be merged then?

gengliangwang commented 6 years ago

@aa8y there are still two comments need to be addressed.

aa8y commented 6 years ago

@gengliangwang Sorry. I didn't see those. I left responses to the comments. I'll wait for your response before I push something.

aa8y commented 6 years ago

@gengliangwang Sorry for the delay. I pushed a commit to fix the braces for the if-else block.

gengliangwang commented 6 years ago

Thanks, merge to master