ARROW-1816: [Java] Resolve new vector classes structure for timestamp, date and maybe interval #1330

icexelloss · 2017-11-18T22:00:51Z

Overview

This PR includes these major changes:

Consolidate all timestamp vectors into a single vector class
Add support for non-simple minor type (i.e., decimal, timestamp) in Union

The second is needed because currently Union vector doesn't support non-simple minor type such as decimal. Because we make timestamp type also a non-simple minor type, we need this change to support timestamp and decimal in Union.

Proposed API Changes

Merge TimeStamp*Vector to a single class TimestampVector
Rename "timeStamp" to "timestamp" and "TimeStamp" to "Timestamp" across the API for better consistency with Java naming conversions of "time stamp"
- https://docs.oracle.com/javase/7/docs/api/java/sql/Timestamp.html
- https://spark.apache.org/docs/2.2.0/api/java/org/apache/spark/sql/types/TimestampType.html
Merge MinorType.TIMESTAMP* to a single MinorType.TIMESTAMP that doesn't support getType() (same as DECIMAL minorType)
Merge TimeStamp*Writer to a single TimestampWriter
Merge TimeStamp*Reader to a single TimestampReader
Change UnionWriter.writeTimestamp*(long) to UnionWriter.asTimestamp(unit, timezone).writeTimestamp(long)
Change ListWriter.timeStamp*() to ListWriter.timestamp(unit, timezone)
Change MapWriter.timeStamp*(name) to MapWriter.timestamp(name, unit, timezone)
Change UnionVector.getTimeStamp*Vector to UnionVector.getTimestampVector() and UnionVector.getTimestampVector(unit, timezone). UnionVector.getTimestampVector() will return the existing timestamp vector or throw exception if it doesn't exist. UnionVector.getTimestampVector(unit, timezone) will return the existing timestamp vector or create one if it doesn't exist.

Added API

Add UnionVector.getDecimalVector() and UnionVector.getDecimalVector(precision, scale)
Add ListWriter.decimal(precision, scale)
Add MapWriter.decimal(name, precision, scale)
Add UnionWriter.asDecimal(precision, scale) and UnionWriter.getDecimalWriter(precision, scale)

TODO:

Add more test

icexelloss · 2017-11-18T22:03:19Z

This PR is a RFC.

I think the resulting timestamp vector (NullableTimestampVector) is still branch-free at the cell level. cc @jacques-n can you take a look and let me know what you think?

icexelloss · 2017-11-20T15:19:36Z

@wesm @jacques-n @siddharthteotia Timing wise I should be able to make this into 0.8 if we are happy about this approach.

jacques-n · 2017-11-20T16:23:18Z

On first look this makes sense. However, if I'm reading this right, the reader/writer implementations have lost the ability to set the right unit. How does one do this?

icexelloss · 2017-11-20T16:52:59Z

@jacques-n I am fixing that part.

BryanCutler

LGTM as long as there is no performance degredation. My biggest concern is that maybe there are some operations that now need to check for compatible time units.

BryanCutler · 2017-11-21T18:24:54Z

java/vector/src/main/codegen/data/ValueVectorTypes.tdd

These were previously flipped in ARROW-1091 and then flipped again in ARROW-1092. I thought they were right already, are they not?

The reverse ordering the these typeParams are driving me nuts... I changed it such that they follow the same order everywhere. Also see: https://github.com/apache/arrow/pull/1330/files#r152436539

The end results are the same because flipped twice..

BryanCutler · 2017-11-21T18:27:49Z

java/vector/src/main/codegen/data/ValueVectorTypes.tdd

Is the friendlyType param still used any more? If so then will LocalDateTime work with a timezone set?

It is used. The way it currently works is if timezone is specified, if will return the LocalDateTime in the specified time zone.

For example:

value = 0, timezone = null

returns LocalDateTime(1970-01-01 00:00:00)

value = 0, timezone = "America/New_York"

returns LocalDateTime(1969-12-31 19:00:00)

BryanCutler · 2017-11-21T18:28:58Z

java/vector/pom.xml

How come this needs to be added now? If it does can it be scoped to compile phase?

This should just be build dependency. Before it was using 2.3.21.

So without specifying the version it defaults to 2.3.21? Was that version causing an issue?

Yes I think that's the version comes with the plugin.

I used <#sep> notation in the template to created comma separated arg list. The notation is introduced in 2.3.23

BryanCutler · 2017-11-21T18:31:49Z

java/vector/src/main/codegen/templates/MapWriters.java

Why change this?

Making param order more sane.. It's driving me nuts we are flipping the ordering twice.

BryanCutler · 2017-11-21T18:40:31Z

java/vector/src/main/codegen/templates/UnionVector.java

forgot to remove?

BryanCutler · 2017-11-21T18:43:43Z

java/vector/src/main/java/org/apache/arrow/vector/NullableTimestampVector.java

minor: maybe move this import up with DateTimeZone, and move the TimeUnit down with other arrow

We really need that checkstyle :)

BryanCutler · 2017-11-21T18:55:21Z

java/vector/src/main/java/org/apache/arrow/vector/NullableTimestampVector.java

Do these set methods need to check that the type params are compatible?

Good point. Although it does add branching on the array. Maybe don't check for performance reason, not sure.

BryanCutler · 2017-11-21T19:00:28Z

java/vector/src/main/java/org/apache/arrow/vector/types/TimeUnit.java

Would it be better to leave this as it was and just put a switch statement in getTimeUnit to return the corresponding java TimeUnit?

Not sure why that's better...Why do you think so? Maybe I missed sth.

Well before the enum was defined by the flatbufId, but here it is the flatbufId and the Java TimeUnit which is redundant information. So a check for equality would have to look at both of these fields. Maybe not a big deal though..

Enum equality should just be identity check:
https://stackoverflow.com/questions/533922/is-it-ok-to-use-on-enums-in-java

icexelloss · 2017-11-21T23:51:52Z

@jacques-n During the refactoring, I discovered that union vector doesn't support types with type params (i.e. Decimal and Timestamp), so I fixed that as well. Now union vectors support decimal as well.

The PR now includes two major changes:

Consolidate all timestamp vectors into a single vector class
Add support for non-simple minor type (i.e., decimal, timestamp) in Union

Most of the template change is to support types with type params, most of them replaces:

<#if !minor.typeParams?? >
// handle types with no type params
</#if>

to

<#if !minor.typeParams?? >
// handle types with no type params
<#else>
// handle types with type params
</#if>

Please take a look when you have chance. Thanks! Hopefully we can clean this up and merge the week after Thanksgiving.

icexelloss · 2017-11-21T23:53:17Z

@BryanCutler Thanks for the code review. I will clean this up.

icexelloss · 2017-11-22T00:01:58Z

java/vector/src/main/codegen/templates/FixedValueVectors.java

Making type params order more sane...

icexelloss · 2017-11-22T03:55:02Z

@BryanCutler I am a bit reluctant to check unit and timezone in value holders because of performance reasons. This is currently not checked with other type with type params either, such as decimal. We should maybe visit this problem as a whole as follow up?

BryanCutler · 2017-11-22T16:48:41Z

@BryanCutler I am a bit reluctant to check unit and timezone in value holders because of performance reasons.

Yeah it doesn't make sense to do all these checks each access, so I just wanted to pose the question to make sure it wasn't a blocker for doing this refactor. I don't use the holder APIs so it's fine with me but maybe @siddharthteotia has some thoughts on this?

wesm · 2017-11-23T18:40:00Z

java/vector/src/main/java/org/apache/arrow/vector/NullableTimestampVector.java

Would it be more efficient to have a toMillis method on TimeUnit (and encapsulate this detail per law of demeter)? I guess we need to run some microbenchmarks to be able to judge performance

Good point. I encapsulated java TimeUnit detail.

Also I ran a microbench toMilliis for 1B long values. Performance is the same.

wesm · 2017-11-28T17:40:33Z

Needs rebase after ARROW-1710. If we are going to change this class structure, 0.8.0 would be the right time to do it. I'm concerned about delaying the release much further though -- if we can't get the Java work wrapped up this week, we can probably take another week, but we're getting into the red zone as far as the Spark 2.3.0 timeline is concerned

icexelloss · 2017-11-28T21:06:43Z

@wesm I rebased after ARROW-1770. This should be ready.

@jacques-n can you take a look? I'd really like this to get in 0.8 if this looks good. Thanks

BryanCutler · 2017-11-28T23:13:56Z

java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java

Is is possible to just have getWriter(ArrowType)? Having 2 methods so similar is a little confusing.. This is because you need a writer for timestamp type params too?

I agree ideally we should have just one. However currently, this is needed to support this:

TimestampWriter timestampWriter = rootWriter.timestamp("a", TimeUnit.SECOND, "America/New_York"); timestampWriter.writeTimestamp(1000)

The second line here, the type of "timestampWriter" is a promotable writer and it needs to lookup the writer by MinorType.TIMESTAMP

We need to do more refactoring to support this API without abstract protected FieldWriter getWriter(MinorType type), but since this is going to be internal change and this PR is already quite complicated, I prefer this to be a follow up.

Ok, sounds good

compiles. Need to fix tests. Fix tests Tests passed Fix error Remove println Rebase Fix union reader/writer. Add more tests Add testTimestampVector test; Modify unionWriter template Hide java TimeUnit in arrow TimeUnit

siddharthteotia mentioned this pull request Nov 20, 2017

ARROW-1047: [Java] Add Generic Reader Interface for Stream Format #1259

Closed

icexelloss force-pushed the timestamp-vector-refactor branch from f10b834 to 1e87e9d Compare November 21, 2017 01:22

BryanCutler approved these changes Nov 21, 2017

View reviewed changes

BryanCutler mentioned this pull request Nov 21, 2017

ARROW-1710: [Java] Remove Non-Nullable Vectors #1341

Closed

icexelloss commented Nov 22, 2017

View reviewed changes

java/vector/src/main/codegen/templates/FixedValueVectors.java Outdated

Copy link

Contributor Author

icexelloss Nov 22, 2017

Choose a reason for hiding this comment

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

Making type params order more sane...

icexelloss changed the title ~~wip: ARROW-1816: [Java] Resolve new vector classes structure for timestamp, date and maybe interval~~ ARROW-1816: [Java] Resolve new vector classes structure for timestamp, date and maybe interval Nov 22, 2017

icexelloss force-pushed the timestamp-vector-refactor branch from dc04f92 to 9732486 Compare November 22, 2017 19:34

wesm reviewed Nov 23, 2017

View reviewed changes

icexelloss force-pushed the timestamp-vector-refactor branch from 9ab65d5 to b20f3d0 Compare November 28, 2017 19:57

BryanCutler reviewed Nov 28, 2017

View reviewed changes

wesm force-pushed the master branch from 5220283 to 1fd3457 Compare December 1, 2017 16:50

icexelloss added 2 commits December 1, 2017 11:54

Init commit

e79792a

compiles. Need to fix tests. Fix tests Tests passed Fix error Remove println Rebase Fix union reader/writer. Add more tests Add testTimestampVector test; Modify unionWriter template Hide java TimeUnit in arrow TimeUnit

Clean up

6c33285

wesm force-pushed the timestamp-vector-refactor branch from c04f15b to 6c33285 Compare December 1, 2017 16:54

icexelloss closed this Dec 7, 2017

ARROW-1816: [Java] Resolve new vector classes structure for timestamp, date and maybe interval #1330

ARROW-1816: [Java] Resolve new vector classes structure for timestamp, date and maybe interval #1330

Uh oh!

Conversation

icexelloss commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Proposed API Changes

Added API

TODO:

Uh oh!

icexelloss commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

icexelloss commented Nov 20, 2017

Uh oh!

jacques-n commented Nov 20, 2017

Uh oh!

icexelloss commented Nov 20, 2017

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icexelloss Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icexelloss commented Nov 21, 2017

Uh oh!

icexelloss commented Nov 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icexelloss commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryanCutler commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Nov 28, 2017

Uh oh!

icexelloss commented Nov 18, 2017 •

edited

Loading

icexelloss commented Nov 18, 2017 •

edited

Loading

icexelloss Nov 21, 2017 •

edited

Loading

icexelloss commented Nov 22, 2017 •

edited

Loading

BryanCutler Nov 28, 2017 •

edited

Loading

icexelloss Nov 29, 2017 •

edited

Loading