ARROW-835: [Format][C++][Java] Create a new Duration type

[emkornfield]

• Create a new DurationInterval type in format.
• Implement all interval types in C++ and Java including integration test.

Once this is checked in, I think https://issues.apache.org/jira/browse/ARROW-352 can be resolved as a won't fix?

[codecov-io]

Codecov Report

Merging #3644 into master will decrease coverage by <.01%.
The diff coverage is 84.24%.

@@ Coverage Diff @@## master #3644 +/- ##
==========================================
- Coverage 88.07% 88.06% -0.01% 
==========================================
Files 773 774 +1 Lines 96887 97162 +275 Branches 1251 1251 ==========================================
+ Hits 85329 85566 +237 - Misses 11322 11360 +38 
Partials 236 236

Impacted Files	Coverage Δ
cpp/src/arrow/ipc/read-write-test.cc	99.81% <ø> (ø)	⬆️
cpp/src/arrow/ipc/json-test.cc	98.46% <ø> (ø)	⬆️
cpp/src/arrow/visitor.h	50% <ø> (ø)	⬆️
cpp/src/arrow/scalar.h	88.23% <ø> (ø)	⬆️
cpp/src/arrow/array/builder_primitive.h	95.86% <ø> (ø)	⬆️
cpp/src/arrow/type_traits.h	85.71% <0%> (-7.25%)	⬇️
cpp/src/arrow/array/builder_dict.cc	86.06% <0%> (-1.44%)	⬇️
cpp/src/arrow/pretty_print.cc	83.48% <0%> (-1.6%)	⬇️
cpp/src/arrow/visitor.cc	0% <0%> (ø)	⬆️
cpp/src/arrow/type-test.cc	99.21% <100%> (+0.05%)	⬆️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca6fe8...973d80f. Read the comment docs.

[emkornfield]

@wesm could you look at the format changes to see if you are ok with them?
@pravindra Can you look at the impact on the java side (I added all the necessary java classes I think) to confirm this is acceptable for the Java?

[emkornfield]

Failure was NodeJs

[pravindra]

since this is a format change (backward compatible though), I'll wait for @wesm to approve too before merging.

[emkornfield]

@pravindra thanks for the review

[emkornfield]

Looking over the old e-mail threads it might be that TimeDelta or TimestampDelta is a better name for this type?

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm based on comments on the other JIRAs CLs it seems like it would be desirable to add an implementation for C++, I can add that to this CL but would you mind taking a look to see if you are ok with naming/modeling (I would prefer not to have to refactor just due to naming issues if possible).

[emkornfield]

I believe the Python on OS/X error is spurious (it was green on previous build).

[wesm]

Will work on reviewing the C++ side of this

[wesm]

The C++ side of this, and the changes to the Flatbuffers files, all looks good to me. Thanks @emkornfield for slogging through this and implementing C++, Java, and integration tests in one go!

One question I have is whether we want to commit to the name "DurationInterval" rather than alternatives such as "Duration" or "TimeDelta" or "TimeDuration". I don't think the names are sacred

Can someone (@siddharthteotia ?) carve out a little time to review the Java so we can try to get this merged soon?

[wesm]

Do you think it's possible to collapse this into a single class? There was an effort to do this for Timestamp (with a TimeUnit class) but it failed only because Dremio had taken on dependency with the TimestampUNIT classes

[emkornfield]

Potentially, I followed the pattern already established in the codebase, but I'm open to trying if it is desirable. @pravindra@siddharthteotia do you think this is desirable/possible?

[jacques-n]

Sounds okay to me. Let's see what @siddharthteotia and @pravindra say.

[pravindra]

IMO, having a single class is definitely desirable. Single this one is starting on a fresh slate, it's worth a try.

[wesm]

FWIW, I'm a bit disappointed that we aren't deprecating this type =/

[emkornfield]

I didn't want to get dragged into what it means deprecate this, should be easy enough to do if we want it. As noted below I tried to say it was optional which I seem to recall seeing this was ok on a previous ML thread.

[wesm]

Is this redundant with type_fwd.h?

[emkornfield]

Yes, it appears so, I'll push a commit removing them. It looks like there might be more redundancy in builder_primitive.h

[wesm]

I think static_cast (or checked_cast) is sufficient here?

[emkornfield]

done

[wesm]

nit: as far as naming I could see an argument for using

• interval_duration
• interval_day_time
• interval_month

but I'm OK with the way things are here

[emkornfield]

I could see this to, but I'm happier without the reverse notation.

[emkornfield]

note "duration" conflicts with some of our time classes so I renamed to "duration_type", hopefully that is ok

[wesm]

"some of our time classes"?

[emkornfield]

Need to look back I think it was in the vendored time library we were using. I can look deeper if you don't think there should be a conflict

[emkornfield]

looking again they shouldn't conflict, I will try to fix this today.

[wesm]

Awesome, thanks.

[emkornfield]

renamed. had to touch some of the vendored code.

[wesm]

Got it. If it isn't being deprecated then it's reasonable to implement it in C++ and have integration tests

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm Thanks for the review I'll try to address comments in the next few days. In terms of naming, I don't know the etiquette for changing the name once it gets approved on the ML. If was going to change it, I think Duration would be my choice.

@pravindra already reviewed most of the java code, the main changes that happened where plumbing through support for the integration test.

[kou]

Just for your information.

These changes will be conflicted with #4255.
If #4255 is merged, we will be able to just remove these changes.

[emkornfield]

Thanks, they were removed.

[emkornfield]

@wesm I think I addressed all your feedback. The OS/X build seems to in an infinite CMake loop with something like

 the following targets are still affected by RPATH settings:
arrow_python_shared
arrow_shared
arrow_testing_shared
parquet_shared
plasma_shared

I'm not sure why my change would have caused this. Any thoughts?

@pravindra I was able to consolidate all the java vectors into one, could you take a look?

[wesm]

Baffling

[wesm]

It must have something to do with this being an older PR. Is Appveyor enabled on your fork? If not I can push this to my fork to get an Appveyor build going

[emkornfield]

No appveyor isn't enabled on my fork. let me try squashing everything, see if that helps.

[emkornfield]

Ahh, squash seems to have worked, lets hope things stay green.

[wesm]

@emkornfield it looks you made some changes to vendored code which I think we should try to avoid doing in general, can we open a JIRA to restore to the vendored version and address the name conflict another way? I don't want this issue to block the patch since I need this to go in and then rebase #4316 on top

[wesm]

Appveyor appears to be about ~6 hours backed up so I'm going to push a branch on my fork to try to get a quicker thumbs up to merge this

[wesm]

Here's an Appveyor build starting now on my fork which will take a while to run https://ci.appveyor.com/project/wesm/arrow/builds/24577591

[emkornfield]

Thanks. https://issues.apache.org/jira/browse/ARROW-5346 opened to revert the changes. Right now the only solution seems like reordering includes in some cases or fixing upstream. I will have to think about this.

[wesm]

Appveyor build looks OK, I'm going to wait a little while yet to make sure the MinGW build works before merging

[emkornfield]

@wesm appveyor on your branch looks all green.

[wesm]

Awesome. Thanks @emkornfield!!

[emkornfield]

• Create a new DurationInterval type in format.
• Implement all interval types in C++ and Java including integration test.

Once this is checked in, I think https://issues.apache.org/jira/browse/ARROW-352 can be resolved as a won't fix?

[codecov-io]

Codecov Report

Merging #3644 into master will decrease coverage by <.01%.
The diff coverage is 84.24%.

@@ Coverage Diff @@## master #3644 +/- ##
==========================================
- Coverage 88.07% 88.06% -0.01% 
==========================================
Files 773 774 +1 Lines 96887 97162 +275 Branches 1251 1251 ==========================================
+ Hits 85329 85566 +237 - Misses 11322 11360 +38 
Partials 236 236

Impacted Files	Coverage Δ
cpp/src/arrow/ipc/read-write-test.cc	99.81% <ø> (ø)	⬆️
cpp/src/arrow/ipc/json-test.cc	98.46% <ø> (ø)	⬆️
cpp/src/arrow/visitor.h	50% <ø> (ø)	⬆️
cpp/src/arrow/scalar.h	88.23% <ø> (ø)	⬆️
cpp/src/arrow/array/builder_primitive.h	95.86% <ø> (ø)	⬆️
cpp/src/arrow/type_traits.h	85.71% <0%> (-7.25%)	⬇️
cpp/src/arrow/array/builder_dict.cc	86.06% <0%> (-1.44%)	⬇️
cpp/src/arrow/pretty_print.cc	83.48% <0%> (-1.6%)	⬇️
cpp/src/arrow/visitor.cc	0% <0%> (ø)	⬆️
cpp/src/arrow/type-test.cc	99.21% <100%> (+0.05%)	⬆️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca6fe8...973d80f. Read the comment docs.

[emkornfield]

@wesm could you look at the format changes to see if you are ok with them?
@pravindra Can you look at the impact on the java side (I added all the necessary java classes I think) to confirm this is acceptable for the Java?

[emkornfield]

Failure was NodeJs

[pravindra]

since this is a format change (backward compatible though), I'll wait for @wesm to approve too before merging.

[emkornfield]

@pravindra thanks for the review

[emkornfield]

Looking over the old e-mail threads it might be that TimeDelta or TimestampDelta is a better name for this type?

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm based on comments on the other JIRAs CLs it seems like it would be desirable to add an implementation for C++, I can add that to this CL but would you mind taking a look to see if you are ok with naming/modeling (I would prefer not to have to refactor just due to naming issues if possible).

[emkornfield]

I believe the Python on OS/X error is spurious (it was green on previous build).

[wesm]

Will work on reviewing the C++ side of this

[wesm]

The C++ side of this, and the changes to the Flatbuffers files, all looks good to me. Thanks @emkornfield for slogging through this and implementing C++, Java, and integration tests in one go!

One question I have is whether we want to commit to the name "DurationInterval" rather than alternatives such as "Duration" or "TimeDelta" or "TimeDuration". I don't think the names are sacred

Can someone (@siddharthteotia ?) carve out a little time to review the Java so we can try to get this merged soon?

[wesm]

Do you think it's possible to collapse this into a single class? There was an effort to do this for Timestamp (with a TimeUnit class) but it failed only because Dremio had taken on dependency with the TimestampUNIT classes

[emkornfield]

Potentially, I followed the pattern already established in the codebase, but I'm open to trying if it is desirable. @pravindra@siddharthteotia do you think this is desirable/possible?

[jacques-n]

Sounds okay to me. Let's see what @siddharthteotia and @pravindra say.

[pravindra]

IMO, having a single class is definitely desirable. Single this one is starting on a fresh slate, it's worth a try.

[wesm]

FWIW, I'm a bit disappointed that we aren't deprecating this type =/

[emkornfield]

I didn't want to get dragged into what it means deprecate this, should be easy enough to do if we want it. As noted below I tried to say it was optional which I seem to recall seeing this was ok on a previous ML thread.

[wesm]

Is this redundant with type_fwd.h?

[emkornfield]

Yes, it appears so, I'll push a commit removing them. It looks like there might be more redundancy in builder_primitive.h

[wesm]

I think static_cast (or checked_cast) is sufficient here?

[emkornfield]

done

[wesm]

nit: as far as naming I could see an argument for using

• interval_duration
• interval_day_time
• interval_month

but I'm OK with the way things are here

[emkornfield]

I could see this to, but I'm happier without the reverse notation.

[emkornfield]

note "duration" conflicts with some of our time classes so I renamed to "duration_type", hopefully that is ok

[wesm]

"some of our time classes"?

[emkornfield]

Need to look back I think it was in the vendored time library we were using. I can look deeper if you don't think there should be a conflict

[emkornfield]

looking again they shouldn't conflict, I will try to fix this today.

[wesm]

Awesome, thanks.

[emkornfield]

renamed. had to touch some of the vendored code.

[wesm]

Got it. If it isn't being deprecated then it's reasonable to implement it in C++ and have integration tests

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm Thanks for the review I'll try to address comments in the next few days. In terms of naming, I don't know the etiquette for changing the name once it gets approved on the ML. If was going to change it, I think Duration would be my choice.

@pravindra already reviewed most of the java code, the main changes that happened where plumbing through support for the integration test.

[kou]

Just for your information.

These changes will be conflicted with #4255.
If #4255 is merged, we will be able to just remove these changes.

[emkornfield]

Thanks, they were removed.

[emkornfield]

@wesm I think I addressed all your feedback. The OS/X build seems to in an infinite CMake loop with something like

 the following targets are still affected by RPATH settings:
arrow_python_shared
arrow_shared
arrow_testing_shared
parquet_shared
plasma_shared

I'm not sure why my change would have caused this. Any thoughts?

@pravindra I was able to consolidate all the java vectors into one, could you take a look?

[wesm]

Baffling

[wesm]

It must have something to do with this being an older PR. Is Appveyor enabled on your fork? If not I can push this to my fork to get an Appveyor build going

[emkornfield]

No appveyor isn't enabled on my fork. let me try squashing everything, see if that helps.

[emkornfield]

Ahh, squash seems to have worked, lets hope things stay green.

[wesm]

@emkornfield it looks you made some changes to vendored code which I think we should try to avoid doing in general, can we open a JIRA to restore to the vendored version and address the name conflict another way? I don't want this issue to block the patch since I need this to go in and then rebase #4316 on top

[wesm]

Appveyor appears to be about ~6 hours backed up so I'm going to push a branch on my fork to try to get a quicker thumbs up to merge this

[wesm]

Here's an Appveyor build starting now on my fork which will take a while to run https://ci.appveyor.com/project/wesm/arrow/builds/24577591

[emkornfield]

Thanks. https://issues.apache.org/jira/browse/ARROW-5346 opened to revert the changes. Right now the only solution seems like reordering includes in some cases or fixing upstream. I will have to think about this.

[wesm]

Appveyor build looks OK, I'm going to wait a little while yet to make sure the MinGW build works before merging

[emkornfield]

@wesm appveyor on your branch looks all green.

[wesm]

Awesome. Thanks @emkornfield!!

[emkornfield]

• Create a new DurationInterval type in format.
• Implement all interval types in C++ and Java including integration test.

Once this is checked in, I think https://issues.apache.org/jira/browse/ARROW-352 can be resolved as a won't fix?

[codecov-io]

Codecov Report

Merging #3644 into master will decrease coverage by <.01%.
The diff coverage is 84.24%.

@@ Coverage Diff @@## master #3644 +/- ##
==========================================
- Coverage 88.07% 88.06% -0.01% 
==========================================
Files 773 774 +1 Lines 96887 97162 +275 Branches 1251 1251 ==========================================
+ Hits 85329 85566 +237 - Misses 11322 11360 +38 
Partials 236 236

Impacted Files	Coverage Δ
cpp/src/arrow/ipc/read-write-test.cc	99.81% <ø> (ø)	⬆️
cpp/src/arrow/ipc/json-test.cc	98.46% <ø> (ø)	⬆️
cpp/src/arrow/visitor.h	50% <ø> (ø)	⬆️
cpp/src/arrow/scalar.h	88.23% <ø> (ø)	⬆️
cpp/src/arrow/array/builder_primitive.h	95.86% <ø> (ø)	⬆️
cpp/src/arrow/type_traits.h	85.71% <0%> (-7.25%)	⬇️
cpp/src/arrow/array/builder_dict.cc	86.06% <0%> (-1.44%)	⬇️
cpp/src/arrow/pretty_print.cc	83.48% <0%> (-1.6%)	⬇️
cpp/src/arrow/visitor.cc	0% <0%> (ø)	⬆️
cpp/src/arrow/type-test.cc	99.21% <100%> (+0.05%)	⬆️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca6fe8...973d80f. Read the comment docs.

[emkornfield]

@wesm could you look at the format changes to see if you are ok with them?
@pravindra Can you look at the impact on the java side (I added all the necessary java classes I think) to confirm this is acceptable for the Java?

[emkornfield]

Failure was NodeJs

[pravindra]

since this is a format change (backward compatible though), I'll wait for @wesm to approve too before merging.

[emkornfield]

@pravindra thanks for the review

[emkornfield]

Looking over the old e-mail threads it might be that TimeDelta or TimestampDelta is a better name for this type?

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm based on comments on the other JIRAs CLs it seems like it would be desirable to add an implementation for C++, I can add that to this CL but would you mind taking a look to see if you are ok with naming/modeling (I would prefer not to have to refactor just due to naming issues if possible).

[emkornfield]

I believe the Python on OS/X error is spurious (it was green on previous build).

[wesm]

Will work on reviewing the C++ side of this

[wesm]

The C++ side of this, and the changes to the Flatbuffers files, all looks good to me. Thanks @emkornfield for slogging through this and implementing C++, Java, and integration tests in one go!

One question I have is whether we want to commit to the name "DurationInterval" rather than alternatives such as "Duration" or "TimeDelta" or "TimeDuration". I don't think the names are sacred

Can someone (@siddharthteotia ?) carve out a little time to review the Java so we can try to get this merged soon?

[wesm]

Do you think it's possible to collapse this into a single class? There was an effort to do this for Timestamp (with a TimeUnit class) but it failed only because Dremio had taken on dependency with the TimestampUNIT classes

[emkornfield]

Potentially, I followed the pattern already established in the codebase, but I'm open to trying if it is desirable. @pravindra@siddharthteotia do you think this is desirable/possible?

[jacques-n]

Sounds okay to me. Let's see what @siddharthteotia and @pravindra say.

[pravindra]

IMO, having a single class is definitely desirable. Single this one is starting on a fresh slate, it's worth a try.

[wesm]

FWIW, I'm a bit disappointed that we aren't deprecating this type =/

[emkornfield]

I didn't want to get dragged into what it means deprecate this, should be easy enough to do if we want it. As noted below I tried to say it was optional which I seem to recall seeing this was ok on a previous ML thread.

[wesm]

Is this redundant with type_fwd.h?

[emkornfield]

Yes, it appears so, I'll push a commit removing them. It looks like there might be more redundancy in builder_primitive.h

[wesm]

I think static_cast (or checked_cast) is sufficient here?

[emkornfield]

done

[wesm]

nit: as far as naming I could see an argument for using

• interval_duration
• interval_day_time
• interval_month

but I'm OK with the way things are here

[emkornfield]

I could see this to, but I'm happier without the reverse notation.

[emkornfield]

note "duration" conflicts with some of our time classes so I renamed to "duration_type", hopefully that is ok

[wesm]

"some of our time classes"?

[emkornfield]

Need to look back I think it was in the vendored time library we were using. I can look deeper if you don't think there should be a conflict

[emkornfield]

looking again they shouldn't conflict, I will try to fix this today.

[wesm]

Awesome, thanks.

[emkornfield]

renamed. had to touch some of the vendored code.

[wesm]

Got it. If it isn't being deprecated then it's reasonable to implement it in C++ and have integration tests

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm Thanks for the review I'll try to address comments in the next few days. In terms of naming, I don't know the etiquette for changing the name once it gets approved on the ML. If was going to change it, I think Duration would be my choice.

@pravindra already reviewed most of the java code, the main changes that happened where plumbing through support for the integration test.

[kou]

Just for your information.

These changes will be conflicted with #4255.
If #4255 is merged, we will be able to just remove these changes.

[emkornfield]

Thanks, they were removed.

[emkornfield]

@wesm I think I addressed all your feedback. The OS/X build seems to in an infinite CMake loop with something like

 the following targets are still affected by RPATH settings:
arrow_python_shared
arrow_shared
arrow_testing_shared
parquet_shared
plasma_shared

I'm not sure why my change would have caused this. Any thoughts?

@pravindra I was able to consolidate all the java vectors into one, could you take a look?

[wesm]

Baffling

[wesm]

It must have something to do with this being an older PR. Is Appveyor enabled on your fork? If not I can push this to my fork to get an Appveyor build going

[emkornfield]

No appveyor isn't enabled on my fork. let me try squashing everything, see if that helps.

[emkornfield]

Ahh, squash seems to have worked, lets hope things stay green.

[wesm]

@emkornfield it looks you made some changes to vendored code which I think we should try to avoid doing in general, can we open a JIRA to restore to the vendored version and address the name conflict another way? I don't want this issue to block the patch since I need this to go in and then rebase #4316 on top

[wesm]

Appveyor appears to be about ~6 hours backed up so I'm going to push a branch on my fork to try to get a quicker thumbs up to merge this

[wesm]

Here's an Appveyor build starting now on my fork which will take a while to run https://ci.appveyor.com/project/wesm/arrow/builds/24577591

[emkornfield]

Thanks. https://issues.apache.org/jira/browse/ARROW-5346 opened to revert the changes. Right now the only solution seems like reordering includes in some cases or fixing upstream. I will have to think about this.

[wesm]

Appveyor build looks OK, I'm going to wait a little while yet to make sure the MinGW build works before merging

[emkornfield]

@wesm appveyor on your branch looks all green.

[wesm]

Awesome. Thanks @emkornfield!!

[emkornfield]

• Create a new DurationInterval type in format.
• Implement all interval types in C++ and Java including integration test.

Once this is checked in, I think https://issues.apache.org/jira/browse/ARROW-352 can be resolved as a won't fix?

[codecov-io]

Codecov Report

Merging #3644 into master will decrease coverage by <.01%.
The diff coverage is 84.24%.

@@ Coverage Diff @@## master #3644 +/- ##
==========================================
- Coverage 88.07% 88.06% -0.01% 
==========================================
Files 773 774 +1 Lines 96887 97162 +275 Branches 1251 1251 ==========================================
+ Hits 85329 85566 +237 - Misses 11322 11360 +38 
Partials 236 236

Impacted Files	Coverage Δ
cpp/src/arrow/ipc/read-write-test.cc	99.81% <ø> (ø)	⬆️
cpp/src/arrow/ipc/json-test.cc	98.46% <ø> (ø)	⬆️
cpp/src/arrow/visitor.h	50% <ø> (ø)	⬆️
cpp/src/arrow/scalar.h	88.23% <ø> (ø)	⬆️
cpp/src/arrow/array/builder_primitive.h	95.86% <ø> (ø)	⬆️
cpp/src/arrow/type_traits.h	85.71% <0%> (-7.25%)	⬇️
cpp/src/arrow/array/builder_dict.cc	86.06% <0%> (-1.44%)	⬇️
cpp/src/arrow/pretty_print.cc	83.48% <0%> (-1.6%)	⬇️
cpp/src/arrow/visitor.cc	0% <0%> (ø)	⬆️
cpp/src/arrow/type-test.cc	99.21% <100%> (+0.05%)	⬆️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca6fe8...973d80f. Read the comment docs.

[emkornfield]

@wesm could you look at the format changes to see if you are ok with them?
@pravindra Can you look at the impact on the java side (I added all the necessary java classes I think) to confirm this is acceptable for the Java?

[emkornfield]

Failure was NodeJs

[pravindra]

since this is a format change (backward compatible though), I'll wait for @wesm to approve too before merging.

[emkornfield]

@pravindra thanks for the review

[emkornfield]

Looking over the old e-mail threads it might be that TimeDelta or TimestampDelta is a better name for this type?

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm based on comments on the other JIRAs CLs it seems like it would be desirable to add an implementation for C++, I can add that to this CL but would you mind taking a look to see if you are ok with naming/modeling (I would prefer not to have to refactor just due to naming issues if possible).

[emkornfield]

I believe the Python on OS/X error is spurious (it was green on previous build).

[wesm]

Will work on reviewing the C++ side of this

[wesm]

The C++ side of this, and the changes to the Flatbuffers files, all looks good to me. Thanks @emkornfield for slogging through this and implementing C++, Java, and integration tests in one go!

One question I have is whether we want to commit to the name "DurationInterval" rather than alternatives such as "Duration" or "TimeDelta" or "TimeDuration". I don't think the names are sacred

Can someone (@siddharthteotia ?) carve out a little time to review the Java so we can try to get this merged soon?

[wesm]

Do you think it's possible to collapse this into a single class? There was an effort to do this for Timestamp (with a TimeUnit class) but it failed only because Dremio had taken on dependency with the TimestampUNIT classes

[emkornfield]

Potentially, I followed the pattern already established in the codebase, but I'm open to trying if it is desirable. @pravindra@siddharthteotia do you think this is desirable/possible?

[jacques-n]

Sounds okay to me. Let's see what @siddharthteotia and @pravindra say.

[pravindra]

IMO, having a single class is definitely desirable. Single this one is starting on a fresh slate, it's worth a try.

[wesm]

FWIW, I'm a bit disappointed that we aren't deprecating this type =/

[emkornfield]

I didn't want to get dragged into what it means deprecate this, should be easy enough to do if we want it. As noted below I tried to say it was optional which I seem to recall seeing this was ok on a previous ML thread.

[wesm]

Is this redundant with type_fwd.h?

[emkornfield]

Yes, it appears so, I'll push a commit removing them. It looks like there might be more redundancy in builder_primitive.h

[wesm]

I think static_cast (or checked_cast) is sufficient here?

[emkornfield]

done

[wesm]

nit: as far as naming I could see an argument for using

• interval_duration
• interval_day_time
• interval_month

but I'm OK with the way things are here

[emkornfield]

I could see this to, but I'm happier without the reverse notation.

[emkornfield]

note "duration" conflicts with some of our time classes so I renamed to "duration_type", hopefully that is ok

[wesm]

"some of our time classes"?

[emkornfield]

Need to look back I think it was in the vendored time library we were using. I can look deeper if you don't think there should be a conflict

[emkornfield]

looking again they shouldn't conflict, I will try to fix this today.

[wesm]

Awesome, thanks.

[emkornfield]

renamed. had to touch some of the vendored code.

[wesm]

Got it. If it isn't being deprecated then it's reasonable to implement it in C++ and have integration tests

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm Thanks for the review I'll try to address comments in the next few days. In terms of naming, I don't know the etiquette for changing the name once it gets approved on the ML. If was going to change it, I think Duration would be my choice.

@pravindra already reviewed most of the java code, the main changes that happened where plumbing through support for the integration test.

[kou]

Just for your information.

These changes will be conflicted with #4255.
If #4255 is merged, we will be able to just remove these changes.

[emkornfield]

Thanks, they were removed.

[emkornfield]

@wesm I think I addressed all your feedback. The OS/X build seems to in an infinite CMake loop with something like

 the following targets are still affected by RPATH settings:
arrow_python_shared
arrow_shared
arrow_testing_shared
parquet_shared
plasma_shared

I'm not sure why my change would have caused this. Any thoughts?

@pravindra I was able to consolidate all the java vectors into one, could you take a look?

[wesm]

Baffling

[wesm]

It must have something to do with this being an older PR. Is Appveyor enabled on your fork? If not I can push this to my fork to get an Appveyor build going

[emkornfield]

No appveyor isn't enabled on my fork. let me try squashing everything, see if that helps.

[emkornfield]

Ahh, squash seems to have worked, lets hope things stay green.

[wesm]

@emkornfield it looks you made some changes to vendored code which I think we should try to avoid doing in general, can we open a JIRA to restore to the vendored version and address the name conflict another way? I don't want this issue to block the patch since I need this to go in and then rebase #4316 on top

[wesm]

Appveyor appears to be about ~6 hours backed up so I'm going to push a branch on my fork to try to get a quicker thumbs up to merge this

[wesm]

Here's an Appveyor build starting now on my fork which will take a while to run https://ci.appveyor.com/project/wesm/arrow/builds/24577591

[emkornfield]

Thanks. https://issues.apache.org/jira/browse/ARROW-5346 opened to revert the changes. Right now the only solution seems like reordering includes in some cases or fixing upstream. I will have to think about this.

[wesm]

Appveyor build looks OK, I'm going to wait a little while yet to make sure the MinGW build works before merging

[emkornfield]

@wesm appveyor on your branch looks all green.

[wesm]

Awesome. Thanks @emkornfield!!

[emkornfield]

• Create a new DurationInterval type in format.
• Implement all interval types in C++ and Java including integration test.

Once this is checked in, I think https://issues.apache.org/jira/browse/ARROW-352 can be resolved as a won't fix?

[codecov-io]

Codecov Report

Merging #3644 into master will decrease coverage by <.01%.
The diff coverage is 84.24%.

@@ Coverage Diff @@## master #3644 +/- ##
==========================================
- Coverage 88.07% 88.06% -0.01% 
==========================================
Files 773 774 +1 Lines 96887 97162 +275 Branches 1251 1251 ==========================================
+ Hits 85329 85566 +237 - Misses 11322 11360 +38 
Partials 236 236

Impacted Files	Coverage Δ
cpp/src/arrow/ipc/read-write-test.cc	99.81% <ø> (ø)	⬆️
cpp/src/arrow/ipc/json-test.cc	98.46% <ø> (ø)	⬆️
cpp/src/arrow/visitor.h	50% <ø> (ø)	⬆️
cpp/src/arrow/scalar.h	88.23% <ø> (ø)	⬆️
cpp/src/arrow/array/builder_primitive.h	95.86% <ø> (ø)	⬆️
cpp/src/arrow/type_traits.h	85.71% <0%> (-7.25%)	⬇️
cpp/src/arrow/array/builder_dict.cc	86.06% <0%> (-1.44%)	⬇️
cpp/src/arrow/pretty_print.cc	83.48% <0%> (-1.6%)	⬇️
cpp/src/arrow/visitor.cc	0% <0%> (ø)	⬆️
cpp/src/arrow/type-test.cc	99.21% <100%> (+0.05%)	⬆️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca6fe8...973d80f. Read the comment docs.

[emkornfield]

@wesm could you look at the format changes to see if you are ok with them?
@pravindra Can you look at the impact on the java side (I added all the necessary java classes I think) to confirm this is acceptable for the Java?

[emkornfield]

Failure was NodeJs

[pravindra]

since this is a format change (backward compatible though), I'll wait for @wesm to approve too before merging.

[emkornfield]

@pravindra thanks for the review

[emkornfield]

Looking over the old e-mail threads it might be that TimeDelta or TimestampDelta is a better name for this type?

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm based on comments on the other JIRAs CLs it seems like it would be desirable to add an implementation for C++, I can add that to this CL but would you mind taking a look to see if you are ok with naming/modeling (I would prefer not to have to refactor just due to naming issues if possible).

[emkornfield]

I believe the Python on OS/X error is spurious (it was green on previous build).

[wesm]

Will work on reviewing the C++ side of this

[wesm]

The C++ side of this, and the changes to the Flatbuffers files, all looks good to me. Thanks @emkornfield for slogging through this and implementing C++, Java, and integration tests in one go!

One question I have is whether we want to commit to the name "DurationInterval" rather than alternatives such as "Duration" or "TimeDelta" or "TimeDuration". I don't think the names are sacred

Can someone (@siddharthteotia ?) carve out a little time to review the Java so we can try to get this merged soon?

[wesm]

Do you think it's possible to collapse this into a single class? There was an effort to do this for Timestamp (with a TimeUnit class) but it failed only because Dremio had taken on dependency with the TimestampUNIT classes

[emkornfield]

Potentially, I followed the pattern already established in the codebase, but I'm open to trying if it is desirable. @pravindra@siddharthteotia do you think this is desirable/possible?

[jacques-n]

Sounds okay to me. Let's see what @siddharthteotia and @pravindra say.

[pravindra]

IMO, having a single class is definitely desirable. Single this one is starting on a fresh slate, it's worth a try.

[wesm]

FWIW, I'm a bit disappointed that we aren't deprecating this type =/

[emkornfield]

I didn't want to get dragged into what it means deprecate this, should be easy enough to do if we want it. As noted below I tried to say it was optional which I seem to recall seeing this was ok on a previous ML thread.

[wesm]

Is this redundant with type_fwd.h?

[emkornfield]

Yes, it appears so, I'll push a commit removing them. It looks like there might be more redundancy in builder_primitive.h

[wesm]

I think static_cast (or checked_cast) is sufficient here?

[emkornfield]

done

[wesm]

nit: as far as naming I could see an argument for using

• interval_duration
• interval_day_time
• interval_month

but I'm OK with the way things are here

[emkornfield]

I could see this to, but I'm happier without the reverse notation.

[emkornfield]

note "duration" conflicts with some of our time classes so I renamed to "duration_type", hopefully that is ok

[wesm]

"some of our time classes"?

[emkornfield]

Need to look back I think it was in the vendored time library we were using. I can look deeper if you don't think there should be a conflict

[emkornfield]

looking again they shouldn't conflict, I will try to fix this today.

[wesm]

Awesome, thanks.

[emkornfield]

renamed. had to touch some of the vendored code.

[wesm]

Got it. If it isn't being deprecated then it's reasonable to implement it in C++ and have integration tests

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm Thanks for the review I'll try to address comments in the next few days. In terms of naming, I don't know the etiquette for changing the name once it gets approved on the ML. If was going to change it, I think Duration would be my choice.

@pravindra already reviewed most of the java code, the main changes that happened where plumbing through support for the integration test.

[kou]

Just for your information.

These changes will be conflicted with #4255.
If #4255 is merged, we will be able to just remove these changes.

[emkornfield]

Thanks, they were removed.

[emkornfield]

@wesm I think I addressed all your feedback. The OS/X build seems to in an infinite CMake loop with something like

 the following targets are still affected by RPATH settings:
arrow_python_shared
arrow_shared
arrow_testing_shared
parquet_shared
plasma_shared

I'm not sure why my change would have caused this. Any thoughts?

@pravindra I was able to consolidate all the java vectors into one, could you take a look?

[wesm]

Baffling

[wesm]

It must have something to do with this being an older PR. Is Appveyor enabled on your fork? If not I can push this to my fork to get an Appveyor build going

[emkornfield]

No appveyor isn't enabled on my fork. let me try squashing everything, see if that helps.

[emkornfield]

Ahh, squash seems to have worked, lets hope things stay green.

[wesm]

@emkornfield it looks you made some changes to vendored code which I think we should try to avoid doing in general, can we open a JIRA to restore to the vendored version and address the name conflict another way? I don't want this issue to block the patch since I need this to go in and then rebase #4316 on top

[wesm]

Appveyor appears to be about ~6 hours backed up so I'm going to push a branch on my fork to try to get a quicker thumbs up to merge this

[wesm]

Here's an Appveyor build starting now on my fork which will take a while to run https://ci.appveyor.com/project/wesm/arrow/builds/24577591

[emkornfield]

Thanks. https://issues.apache.org/jira/browse/ARROW-5346 opened to revert the changes. Right now the only solution seems like reordering includes in some cases or fixing upstream. I will have to think about this.

[wesm]

Appveyor build looks OK, I'm going to wait a little while yet to make sure the MinGW build works before merging

[emkornfield]

@wesm appveyor on your branch looks all green.

[wesm]

Awesome. Thanks @emkornfield!!

[emkornfield]

• Create a new DurationInterval type in format.
• Implement all interval types in C++ and Java including integration test.

Once this is checked in, I think https://issues.apache.org/jira/browse/ARROW-352 can be resolved as a won't fix?

[codecov-io]

Codecov Report

Merging #3644 into master will decrease coverage by <.01%.
The diff coverage is 84.24%.

@@ Coverage Diff @@## master #3644 +/- ##
==========================================
- Coverage 88.07% 88.06% -0.01% 
==========================================
Files 773 774 +1 Lines 96887 97162 +275 Branches 1251 1251 ==========================================
+ Hits 85329 85566 +237 - Misses 11322 11360 +38 
Partials 236 236

Impacted Files	Coverage Δ
cpp/src/arrow/ipc/read-write-test.cc	99.81% <ø> (ø)	⬆️
cpp/src/arrow/ipc/json-test.cc	98.46% <ø> (ø)	⬆️
cpp/src/arrow/visitor.h	50% <ø> (ø)	⬆️
cpp/src/arrow/scalar.h	88.23% <ø> (ø)	⬆️
cpp/src/arrow/array/builder_primitive.h	95.86% <ø> (ø)	⬆️
cpp/src/arrow/type_traits.h	85.71% <0%> (-7.25%)	⬇️
cpp/src/arrow/array/builder_dict.cc	86.06% <0%> (-1.44%)	⬇️
cpp/src/arrow/pretty_print.cc	83.48% <0%> (-1.6%)	⬇️
cpp/src/arrow/visitor.cc	0% <0%> (ø)	⬆️
cpp/src/arrow/type-test.cc	99.21% <100%> (+0.05%)	⬆️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca6fe8...973d80f. Read the comment docs.

[emkornfield]

@wesm could you look at the format changes to see if you are ok with them?
@pravindra Can you look at the impact on the java side (I added all the necessary java classes I think) to confirm this is acceptable for the Java?

[emkornfield]

Failure was NodeJs

[pravindra]

since this is a format change (backward compatible though), I'll wait for @wesm to approve too before merging.

[emkornfield]

@pravindra thanks for the review

[emkornfield]

Looking over the old e-mail threads it might be that TimeDelta or TimestampDelta is a better name for this type?

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm based on comments on the other JIRAs CLs it seems like it would be desirable to add an implementation for C++, I can add that to this CL but would you mind taking a look to see if you are ok with naming/modeling (I would prefer not to have to refactor just due to naming issues if possible).

[emkornfield]

I believe the Python on OS/X error is spurious (it was green on previous build).

[wesm]

Will work on reviewing the C++ side of this

[wesm]

The C++ side of this, and the changes to the Flatbuffers files, all looks good to me. Thanks @emkornfield for slogging through this and implementing C++, Java, and integration tests in one go!

One question I have is whether we want to commit to the name "DurationInterval" rather than alternatives such as "Duration" or "TimeDelta" or "TimeDuration". I don't think the names are sacred

Can someone (@siddharthteotia ?) carve out a little time to review the Java so we can try to get this merged soon?

[wesm]

Do you think it's possible to collapse this into a single class? There was an effort to do this for Timestamp (with a TimeUnit class) but it failed only because Dremio had taken on dependency with the TimestampUNIT classes

[emkornfield]

Potentially, I followed the pattern already established in the codebase, but I'm open to trying if it is desirable. @pravindra@siddharthteotia do you think this is desirable/possible?

[jacques-n]

Sounds okay to me. Let's see what @siddharthteotia and @pravindra say.

[pravindra]

IMO, having a single class is definitely desirable. Single this one is starting on a fresh slate, it's worth a try.

[wesm]

FWIW, I'm a bit disappointed that we aren't deprecating this type =/

[emkornfield]

I didn't want to get dragged into what it means deprecate this, should be easy enough to do if we want it. As noted below I tried to say it was optional which I seem to recall seeing this was ok on a previous ML thread.

[wesm]

Is this redundant with type_fwd.h?

[emkornfield]

Yes, it appears so, I'll push a commit removing them. It looks like there might be more redundancy in builder_primitive.h

[wesm]

I think static_cast (or checked_cast) is sufficient here?

[emkornfield]

done

[wesm]

nit: as far as naming I could see an argument for using

• interval_duration
• interval_day_time
• interval_month

but I'm OK with the way things are here

[emkornfield]

I could see this to, but I'm happier without the reverse notation.

[emkornfield]

note "duration" conflicts with some of our time classes so I renamed to "duration_type", hopefully that is ok

[wesm]

"some of our time classes"?

[emkornfield]

Need to look back I think it was in the vendored time library we were using. I can look deeper if you don't think there should be a conflict

[emkornfield]

looking again they shouldn't conflict, I will try to fix this today.

[wesm]

Awesome, thanks.

[emkornfield]

renamed. had to touch some of the vendored code.

[wesm]

Got it. If it isn't being deprecated then it's reasonable to implement it in C++ and have integration tests

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm Thanks for the review I'll try to address comments in the next few days. In terms of naming, I don't know the etiquette for changing the name once it gets approved on the ML. If was going to change it, I think Duration would be my choice.

@pravindra already reviewed most of the java code, the main changes that happened where plumbing through support for the integration test.

[kou]

Just for your information.

These changes will be conflicted with #4255.
If #4255 is merged, we will be able to just remove these changes.

[emkornfield]

Thanks, they were removed.

[emkornfield]

@wesm I think I addressed all your feedback. The OS/X build seems to in an infinite CMake loop with something like

 the following targets are still affected by RPATH settings:
arrow_python_shared
arrow_shared
arrow_testing_shared
parquet_shared
plasma_shared

I'm not sure why my change would have caused this. Any thoughts?

@pravindra I was able to consolidate all the java vectors into one, could you take a look?

[wesm]

Baffling

[wesm]

It must have something to do with this being an older PR. Is Appveyor enabled on your fork? If not I can push this to my fork to get an Appveyor build going

[emkornfield]

No appveyor isn't enabled on my fork. let me try squashing everything, see if that helps.

[emkornfield]

Ahh, squash seems to have worked, lets hope things stay green.

[wesm]

@emkornfield it looks you made some changes to vendored code which I think we should try to avoid doing in general, can we open a JIRA to restore to the vendored version and address the name conflict another way? I don't want this issue to block the patch since I need this to go in and then rebase #4316 on top

[wesm]

Appveyor appears to be about ~6 hours backed up so I'm going to push a branch on my fork to try to get a quicker thumbs up to merge this

[wesm]

Here's an Appveyor build starting now on my fork which will take a while to run https://ci.appveyor.com/project/wesm/arrow/builds/24577591

[emkornfield]

Thanks. https://issues.apache.org/jira/browse/ARROW-5346 opened to revert the changes. Right now the only solution seems like reordering includes in some cases or fixing upstream. I will have to think about this.

[wesm]

Appveyor build looks OK, I'm going to wait a little while yet to make sure the MinGW build works before merging

[emkornfield]

@wesm appveyor on your branch looks all green.

[wesm]

Awesome. Thanks @emkornfield!!

[emkornfield]

• Create a new DurationInterval type in format.
• Implement all interval types in C++ and Java including integration test.

Once this is checked in, I think https://issues.apache.org/jira/browse/ARROW-352 can be resolved as a won't fix?

[codecov-io]

Codecov Report

Merging #3644 into master will decrease coverage by <.01%.
The diff coverage is 84.24%.

@@ Coverage Diff @@## master #3644 +/- ##
==========================================
- Coverage 88.07% 88.06% -0.01% 
==========================================
Files 773 774 +1 Lines 96887 97162 +275 Branches 1251 1251 ==========================================
+ Hits 85329 85566 +237 - Misses 11322 11360 +38 
Partials 236 236

Impacted Files	Coverage Δ
cpp/src/arrow/ipc/read-write-test.cc	99.81% <ø> (ø)	⬆️
cpp/src/arrow/ipc/json-test.cc	98.46% <ø> (ø)	⬆️
cpp/src/arrow/visitor.h	50% <ø> (ø)	⬆️
cpp/src/arrow/scalar.h	88.23% <ø> (ø)	⬆️
cpp/src/arrow/array/builder_primitive.h	95.86% <ø> (ø)	⬆️
cpp/src/arrow/type_traits.h	85.71% <0%> (-7.25%)	⬇️
cpp/src/arrow/array/builder_dict.cc	86.06% <0%> (-1.44%)	⬇️
cpp/src/arrow/pretty_print.cc	83.48% <0%> (-1.6%)	⬇️
cpp/src/arrow/visitor.cc	0% <0%> (ø)	⬆️
cpp/src/arrow/type-test.cc	99.21% <100%> (+0.05%)	⬆️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca6fe8...973d80f. Read the comment docs.

[emkornfield]

@wesm could you look at the format changes to see if you are ok with them?
@pravindra Can you look at the impact on the java side (I added all the necessary java classes I think) to confirm this is acceptable for the Java?

[emkornfield]

Failure was NodeJs

[pravindra]

since this is a format change (backward compatible though), I'll wait for @wesm to approve too before merging.

[emkornfield]

@pravindra thanks for the review

[emkornfield]

Looking over the old e-mail threads it might be that TimeDelta or TimestampDelta is a better name for this type?

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm based on comments on the other JIRAs CLs it seems like it would be desirable to add an implementation for C++, I can add that to this CL but would you mind taking a look to see if you are ok with naming/modeling (I would prefer not to have to refactor just due to naming issues if possible).

[emkornfield]

I believe the Python on OS/X error is spurious (it was green on previous build).

[wesm]

Will work on reviewing the C++ side of this

[wesm]

The C++ side of this, and the changes to the Flatbuffers files, all looks good to me. Thanks @emkornfield for slogging through this and implementing C++, Java, and integration tests in one go!

One question I have is whether we want to commit to the name "DurationInterval" rather than alternatives such as "Duration" or "TimeDelta" or "TimeDuration". I don't think the names are sacred

Can someone (@siddharthteotia ?) carve out a little time to review the Java so we can try to get this merged soon?

[wesm]

Do you think it's possible to collapse this into a single class? There was an effort to do this for Timestamp (with a TimeUnit class) but it failed only because Dremio had taken on dependency with the TimestampUNIT classes

[emkornfield]

Potentially, I followed the pattern already established in the codebase, but I'm open to trying if it is desirable. @pravindra@siddharthteotia do you think this is desirable/possible?

[jacques-n]

Sounds okay to me. Let's see what @siddharthteotia and @pravindra say.

[pravindra]

IMO, having a single class is definitely desirable. Single this one is starting on a fresh slate, it's worth a try.

[wesm]

FWIW, I'm a bit disappointed that we aren't deprecating this type =/

[emkornfield]

I didn't want to get dragged into what it means deprecate this, should be easy enough to do if we want it. As noted below I tried to say it was optional which I seem to recall seeing this was ok on a previous ML thread.

[wesm]

Is this redundant with type_fwd.h?

[emkornfield]

Yes, it appears so, I'll push a commit removing them. It looks like there might be more redundancy in builder_primitive.h

[wesm]

I think static_cast (or checked_cast) is sufficient here?

[emkornfield]

done

[wesm]

nit: as far as naming I could see an argument for using

• interval_duration
• interval_day_time
• interval_month

but I'm OK with the way things are here

[emkornfield]

I could see this to, but I'm happier without the reverse notation.

[emkornfield]

note "duration" conflicts with some of our time classes so I renamed to "duration_type", hopefully that is ok

[wesm]

"some of our time classes"?

[emkornfield]

Need to look back I think it was in the vendored time library we were using. I can look deeper if you don't think there should be a conflict

[emkornfield]

looking again they shouldn't conflict, I will try to fix this today.

[wesm]

Awesome, thanks.

[emkornfield]

renamed. had to touch some of the vendored code.

[wesm]

Got it. If it isn't being deprecated then it's reasonable to implement it in C++ and have integration tests

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm Thanks for the review I'll try to address comments in the next few days. In terms of naming, I don't know the etiquette for changing the name once it gets approved on the ML. If was going to change it, I think Duration would be my choice.

@pravindra already reviewed most of the java code, the main changes that happened where plumbing through support for the integration test.

[kou]

Just for your information.

These changes will be conflicted with #4255.
If #4255 is merged, we will be able to just remove these changes.

[emkornfield]

Thanks, they were removed.

[emkornfield]

@wesm I think I addressed all your feedback. The OS/X build seems to in an infinite CMake loop with something like

 the following targets are still affected by RPATH settings:
arrow_python_shared
arrow_shared
arrow_testing_shared
parquet_shared
plasma_shared

I'm not sure why my change would have caused this. Any thoughts?

@pravindra I was able to consolidate all the java vectors into one, could you take a look?

[wesm]

Baffling

[wesm]

It must have something to do with this being an older PR. Is Appveyor enabled on your fork? If not I can push this to my fork to get an Appveyor build going

[emkornfield]

No appveyor isn't enabled on my fork. let me try squashing everything, see if that helps.

[emkornfield]

Ahh, squash seems to have worked, lets hope things stay green.

[wesm]

@emkornfield it looks you made some changes to vendored code which I think we should try to avoid doing in general, can we open a JIRA to restore to the vendored version and address the name conflict another way? I don't want this issue to block the patch since I need this to go in and then rebase #4316 on top

[wesm]

Appveyor appears to be about ~6 hours backed up so I'm going to push a branch on my fork to try to get a quicker thumbs up to merge this

[wesm]

Here's an Appveyor build starting now on my fork which will take a while to run https://ci.appveyor.com/project/wesm/arrow/builds/24577591

[emkornfield]

Thanks. https://issues.apache.org/jira/browse/ARROW-5346 opened to revert the changes. Right now the only solution seems like reordering includes in some cases or fixing upstream. I will have to think about this.

[wesm]

Appveyor build looks OK, I'm going to wait a little while yet to make sure the MinGW build works before merging

[emkornfield]

@wesm appveyor on your branch looks all green.

[wesm]

Awesome. Thanks @emkornfield!!

[emkornfield]

• Create a new DurationInterval type in format.
• Implement all interval types in C++ and Java including integration test.

Once this is checked in, I think https://issues.apache.org/jira/browse/ARROW-352 can be resolved as a won't fix?

[codecov-io]

Codecov Report

Merging #3644 into master will decrease coverage by <.01%.
The diff coverage is 84.24%.

@@ Coverage Diff @@## master #3644 +/- ##
==========================================
- Coverage 88.07% 88.06% -0.01% 
==========================================
Files 773 774 +1 Lines 96887 97162 +275 Branches 1251 1251 ==========================================
+ Hits 85329 85566 +237 - Misses 11322 11360 +38 
Partials 236 236

Impacted Files	Coverage Δ
cpp/src/arrow/ipc/read-write-test.cc	99.81% <ø> (ø)	⬆️
cpp/src/arrow/ipc/json-test.cc	98.46% <ø> (ø)	⬆️
cpp/src/arrow/visitor.h	50% <ø> (ø)	⬆️
cpp/src/arrow/scalar.h	88.23% <ø> (ø)	⬆️
cpp/src/arrow/array/builder_primitive.h	95.86% <ø> (ø)	⬆️
cpp/src/arrow/type_traits.h	85.71% <0%> (-7.25%)	⬇️
cpp/src/arrow/array/builder_dict.cc	86.06% <0%> (-1.44%)	⬇️
cpp/src/arrow/pretty_print.cc	83.48% <0%> (-1.6%)	⬇️
cpp/src/arrow/visitor.cc	0% <0%> (ø)	⬆️
cpp/src/arrow/type-test.cc	99.21% <100%> (+0.05%)	⬆️
... and 16 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ca6fe8...973d80f. Read the comment docs.

[emkornfield]

@wesm could you look at the format changes to see if you are ok with them?
@pravindra Can you look at the impact on the java side (I added all the necessary java classes I think) to confirm this is acceptable for the Java?

[emkornfield]

Failure was NodeJs

[pravindra]

since this is a format change (backward compatible though), I'll wait for @wesm to approve too before merging.

[emkornfield]

@pravindra thanks for the review

[emkornfield]

Looking over the old e-mail threads it might be that TimeDelta or TimestampDelta is a better name for this type?

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm based on comments on the other JIRAs CLs it seems like it would be desirable to add an implementation for C++, I can add that to this CL but would you mind taking a look to see if you are ok with naming/modeling (I would prefer not to have to refactor just due to naming issues if possible).

[emkornfield]

I believe the Python on OS/X error is spurious (it was green on previous build).

[wesm]

Will work on reviewing the C++ side of this

[wesm]

The C++ side of this, and the changes to the Flatbuffers files, all looks good to me. Thanks @emkornfield for slogging through this and implementing C++, Java, and integration tests in one go!

One question I have is whether we want to commit to the name "DurationInterval" rather than alternatives such as "Duration" or "TimeDelta" or "TimeDuration". I don't think the names are sacred

Can someone (@siddharthteotia ?) carve out a little time to review the Java so we can try to get this merged soon?

[wesm]

Do you think it's possible to collapse this into a single class? There was an effort to do this for Timestamp (with a TimeUnit class) but it failed only because Dremio had taken on dependency with the TimestampUNIT classes

[emkornfield]

Potentially, I followed the pattern already established in the codebase, but I'm open to trying if it is desirable. @pravindra@siddharthteotia do you think this is desirable/possible?

[jacques-n]

Sounds okay to me. Let's see what @siddharthteotia and @pravindra say.

[pravindra]

IMO, having a single class is definitely desirable. Single this one is starting on a fresh slate, it's worth a try.

[wesm]

FWIW, I'm a bit disappointed that we aren't deprecating this type =/

[emkornfield]

I didn't want to get dragged into what it means deprecate this, should be easy enough to do if we want it. As noted below I tried to say it was optional which I seem to recall seeing this was ok on a previous ML thread.

[wesm]

Is this redundant with type_fwd.h?

[emkornfield]

Yes, it appears so, I'll push a commit removing them. It looks like there might be more redundancy in builder_primitive.h

[wesm]

I think static_cast (or checked_cast) is sufficient here?

[emkornfield]

done

[wesm]

nit: as far as naming I could see an argument for using

• interval_duration
• interval_day_time
• interval_month

but I'm OK with the way things are here

[emkornfield]

I could see this to, but I'm happier without the reverse notation.

[emkornfield]

note "duration" conflicts with some of our time classes so I renamed to "duration_type", hopefully that is ok

[wesm]

"some of our time classes"?

[emkornfield]

Need to look back I think it was in the vendored time library we were using. I can look deeper if you don't think there should be a conflict

[emkornfield]

looking again they shouldn't conflict, I will try to fix this today.

[wesm]

Awesome, thanks.

[emkornfield]

renamed. had to touch some of the vendored code.

[wesm]

Got it. If it isn't being deprecated then it's reasonable to implement it in C++ and have integration tests

[wesm]

Are you happy with "DurationInterval"? It could also be called simply "Duration" or "TimeDuration"

[emkornfield]

@wesm Thanks for the review I'll try to address comments in the next few days. In terms of naming, I don't know the etiquette for changing the name once it gets approved on the ML. If was going to change it, I think Duration would be my choice.

@pravindra already reviewed most of the java code, the main changes that happened where plumbing through support for the integration test.

[kou]

Just for your information.

These changes will be conflicted with #4255.
If #4255 is merged, we will be able to just remove these changes.

[emkornfield]

Thanks, they were removed.

[emkornfield]

@wesm I think I addressed all your feedback. The OS/X build seems to in an infinite CMake loop with something like

 the following targets are still affected by RPATH settings:
arrow_python_shared
arrow_shared
arrow_testing_shared
parquet_shared
plasma_shared

I'm not sure why my change would have caused this. Any thoughts?

@pravindra I was able to consolidate all the java vectors into one, could you take a look?

[wesm]

Baffling

[wesm]

It must have something to do with this being an older PR. Is Appveyor enabled on your fork? If not I can push this to my fork to get an Appveyor build going

[emkornfield]

No appveyor isn't enabled on my fork. let me try squashing everything, see if that helps.

[emkornfield]

Ahh, squash seems to have worked, lets hope things stay green.

[wesm]

@emkornfield it looks you made some changes to vendored code which I think we should try to avoid doing in general, can we open a JIRA to restore to the vendored version and address the name conflict another way? I don't want this issue to block the patch since I need this to go in and then rebase #4316 on top

[wesm]

Appveyor appears to be about ~6 hours backed up so I'm going to push a branch on my fork to try to get a quicker thumbs up to merge this

[wesm]

Here's an Appveyor build starting now on my fork which will take a while to run https://ci.appveyor.com/project/wesm/arrow/builds/24577591

[emkornfield]

Thanks. https://issues.apache.org/jira/browse/ARROW-5346 opened to revert the changes. Right now the only solution seems like reordering includes in some cases or fixing upstream. I will have to think about this.

[wesm]

Appveyor build looks OK, I'm going to wait a little while yet to make sure the MinGW build works before merging

[emkornfield]

@wesm appveyor on your branch looks all green.

[wesm]

Awesome. Thanks @emkornfield!!