ARROW-7301: [Java] Sql type DATE should correspond to DateDayVector #5944
Conversation
Change-Id: I604a6b2db3b0816aec6bc7c117e8172507dbdddd
liyafan82
force-pushed
the
fly_1203_date
branch
from
1e236b1 to
eea8b79
Compare
|
For example, this document gives some descriptions about the DATE type in SQL: |
walterddr
left a comment
walterddr
left a comment
There was a problem hiding this comment.
Hi @liyafan82, thanks for fixing this issue. I have a question regarding the Datetime handling, it might've related to one of the issue I am having. could you please kindly take a look at my comment on the DateConsumer class? thanks -Rong.
| return new ArrowType.Utf8(); | ||
| case Types.DATE: | ||
| return new ArrowType.Date(DateUnit.MILLISECOND); | ||
| return new ArrowType.Date(DateUnit.DAY); |
There was a problem hiding this comment.
Nice catch. Thank you.
| import java.sql.ResultSet; | ||
| import java.sql.SQLException; | ||
| import java.util.Calendar; | ||
| import java.util.Date; |
There was a problem hiding this comment.
should probably use java.sql.Date instead?
There was a problem hiding this comment.
good point. thank you.
|
|
||
| @Override | ||
| public void consume(ResultSet resultSet) throws SQLException { | ||
| Date date = calendar == null ? resultSet.getDate(columnIndexInResultSet) : |
There was a problem hiding this comment.
this might be legacy, but resultSet.getDate returns java.sql.Date instead of java.util.Date.
Seems like there might've been some implicit conversion where we don't want underneath?
There was a problem hiding this comment.
Sure. I agree with you that java.sql.Date is better.
Here java.sql.Date is converted to java.util.Date because the latter is the super class of the former.
@walterddr Thanks a lot for your good points. I have revised the code accordingly. Please take another look. Thank you in advance. |
There was a problem hiding this comment.
use TimeUnit.MILLISECONDS.toDays()?
There was a problem hiding this comment.
Good suggestion. I have revised the code. Thanks.
There was a problem hiding this comment.
are SQL dates guaranteed to have the same range as DateDayVector? (i.e. should you check for overflow?)
There was a problem hiding this comment.
Good suggestion. I have added logic to check overflow.
liyafan82
force-pushed
the
fly_1203_date
branch
from
8833a00 to
5a89b62
Compare
liyafan82
force-pushed
the
fly_1203_date
branch
from
5a89b62 to
be73192
Compare
| /** | ||
| * The number of milli-seconds in a day. | ||
| */ | ||
| public static final long MILLIS_PER_DAY = TimeUnit.DAYS.toMillis(1); |
There was a problem hiding this comment.
Isn't there a TimeUnit.MILLIS.toDays that you can use instead of doing the division?
There was a problem hiding this comment.
Revised accordly. Thanks for the good suggestion.
According to SQL convertion, sql type DATE should correspond to a format of YYYY-MM-DD, without the components for hour/minute/second/millis Therefore, JDBC type DATE should correspond to DateDayVector, with a type width of 4, instead of 8. Closes #5944 from liyafan82/fly_1203_date and squashes the following commits: a6de377 <liyafan82> Remove division in time conversion be73192 <liyafan82> Resolve comments eea8b79 <liyafan82> Sql type DATE should correspond to DateDayVector Authored-by: liyafan82 <fan_li_ya@foxmail.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
According to SQL convertion, sql type DATE should correspond to a format of YYYY-MM-DD, without the components for hour/minute/second/millis Therefore, JDBC type DATE should correspond to DateDayVector, with a type width of 4, instead of 8. Closes apache#5944 from liyafan82/fly_1203_date and squashes the following commits: a6de377 <liyafan82> Remove division in time conversion be73192 <liyafan82> Resolve comments eea8b79 <liyafan82> Sql type DATE should correspond to DateDayVector Authored-by: liyafan82 <fan_li_ya@foxmail.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
4 participants
According to SQL convertion, sql type DATE should correspond to a format of YYYY-MM-DD, without the components for hour/minute/second/millis
Therefore, JDBC type DATE should correspond to DateDayVector, with a type width of 4, instead of 8.