Add Support for Intervals #258

WillAyd · 2023-07-17T19:10:39Z

No description provided.

lidavidm · 2023-07-17T19:14:07Z

src/nanoarrow/nanoarrow_types.h

+ enum ArrowIntervalUnit unit;
+
+ /// \brief Pointer containing the underlying data for the interval
+ void* data;


Possibly a union is preferable here to avoid an extra pointer indirection?

codecov-commenter · 2023-07-17T19:15:47Z

Codecov Report

Merging #258 (9e37cc3) into main (a209bab) will decrease coverage by 0.03%.
The diff coverage is 84.84%.

@@            Coverage Diff             @@
##             main     #258      +/-   ##
==========================================
- Coverage   87.17%   87.15%   -0.03%     
==========================================
  Files          66       63       -3     
  Lines       10061     9753     -308     
==========================================
- Hits         8771     8500     -271     
+ Misses       1290     1253      -37

Impacted Files	Coverage Δ
src/nanoarrow/nanoarrow.h	`100.00% <ø> (ø)`
src/nanoarrow/array_inline.h	`89.31% <80.76%> (-0.41%)`	⬇️
src/nanoarrow/nanoarrow_types.h	`93.10% <100.00%> (+0.34%)`	⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

paleolimbot

This is awesome! I have one suggestion about simplifying the ArrowInterval but am excited to have support for this!

src/nanoarrow/array_test.cc

src/nanoarrow/nanoarrow.h

paleolimbot · 2023-07-17T20:06:14Z

src/nanoarrow/nanoarrow_types.h

+/// \brief A representation of an interval.
+struct ArrowInterval {
+ /// \brief The type of interval being used
+ enum ArrowIntervalUnit unit;
+
+ /// \brief Pointer containing the underlying data for the interval
+ union {
+ int32_t year_month;
+ struct ArrowIntervalDayTime day_time;
+ struct ArrowIntervalMonthDayNano month_day_nano;
+ } data;
+};


For ArrowDecimal we do something more along the lines of "declare more fields than needed since the caller probably stack-allocated one of these for whatever loop they're in". The definition here is OK too, but for the sake of argument:

struct ArrowInterval { enum ArrowType type; int32_t month; int32_t day; int64_t time; int64_t nano; };

...might be simpler?

time is supposed to represent milliseconds in that struct right? Not a strong preference but I think using the unions is a little easier as an end user. With the current design I was thinking an anonymous union would be the best, though that would require C11 instead of just C99

For the time being can we parallel the ArrowDecimal pattern (i.e., one flat struct + an ArrowIntervalInit() helper) for consistency? There are almost certainly better names than month/day/type/nano that perhaps give some hint as to how the spec defines them.

Yea makes sense. Actually looking in CI I don't think Cython supports using a union within a struct, so this is probably the only way

Even with flattening to one struct I think we will still need to keep structs like ArrowIntervalDayTime and ArrowIntervalMonthDayNano to unpack the data buffer - is that in line with your expectation or am I misinterpreting?

Ah, now I see reinterpret_cast<const struct ArrowIntervalMonthDayNano*> and friends. In #214 we opted to memcpy() instead of reinterpret_cast<> ( #214 (comment) ), although I'm aware we reinterpret_cast<> and make it easy for others to do so with the int/float/double primitives. I think the idea was to not continue that for anything more exotic than an int64.

I suppose we still need something to memcpy() into though...I'm much more interested in having interval support than having the struct be exactly like ArrowDecimal (although ArrowIntervalUnit can probably just be ArrowType unless there's another distinction I'm missing?).

Nice catch - simplified that

WillAyd · 2023-07-18T00:06:43Z

Any idea what I might be overlooking with the tests and how they allocate memory? Locally I get this valgrind traceback:

==710574==    at 0x4843828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==710574==    by 0x1C6BFD: ArrowMalloc (utils.c:194)
==710574==    by 0x1BF55A: ArrowArrayInitFromType (array.c:139)
==710574==    by 0x143D36: ArrayTest_ArrayTestAppendToIntervalArrayMonthDayNano_Test::TestBody() (array_test.cc:934)
==710574==    by 0x2048CA: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2607)
==710574==    by 0x1FC940: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2643)
==710574==    by 0x1D002F: testing::Test::Run() (gtest.cc:2682)
==710574==    by 0x1D0AFD: testing::TestInfo::Run() (gtest.cc:2861)
==710574==    by 0x1D1426: testing::TestSuite::Run() (gtest.cc:3015)
==710574==    by 0x1E13F4: testing::internal::UnitTestImpl::RunAllTests() (gtest.cc:5855)
==710574==    by 0x2058C9: bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (gtest.cc:2607)
==710574==    by 0x1FDAD2: bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (gtest.cc:2643)

Not immediately clear what is different from the other tests in terms of building the array

WillAyd · 2023-07-18T00:43:35Z

Ignore comment above - I see that ImportArray takes ownership of the memory so probably have to implement that or explicitly call array.release

paleolimbot · 2023-07-18T01:03:28Z

Yes, you'll need to call array.release(&array) at the end of all three tests you added. A better design would have been to use the C++ helpers (i.e., nanoarrow::UniqueArray and friends); however, I didn't have those when I started writing the tests.

paleolimbot · 2023-07-18T01:14:44Z

The Python issue is because we auto-generate the .pxd file from the nanoarrow.h header and in Cython you have to declare the union separately (see https://github.com/apache/arrow-nanoarrow/blob/main/src/nanoarrow/nanoarrow_types.h#L483-L496 ).

WillAyd · 2023-07-19T03:20:19Z

I don't think the Python error is related?

paleolimbot

Yes, the Python failure is happening on main too (maybe Cython 3 releated).

Just a few nits!

paleolimbot · 2023-07-19T13:22:53Z

src/nanoarrow/nanoarrow_types.h

+static inline void ArrowIntervalInit(struct ArrowInterval* interval, enum ArrowType unit,
+ int32_t months, int32_t days, int32_t ms,
+ int64_t ns) {
+ interval->unit = unit;
+ interval->months = months;
+ interval->days = days;
+ interval->ms = ms;
+ interval->ns = ns;
+}


I think this can just zero-initialize and set unit and encourage direct field access like ArrowDecimal? Most users won't use all the fields at once.

paleolimbot · 2023-07-19T13:25:35Z

src/nanoarrow/nanoarrow_types.h

+ int32_t days;
+ int32_t ms;
+ int64_t ns;
+};


Awesome!

I think this will need the fields documented for them to show up in the generated documentation (and \ingroup nanoarrow-utils for it to show up in the right place).

We have a lot of ArrowType variables that are named type...would that be appropriate here?

paleolimbot · 2023-07-19T13:29:45Z

dist/nanoarrow.h

Can you revert the change to this file? I should add the hook to modify it to pre-commit but right now it's updated via a nightly job (for better or worse).

As noted in #258, the Python tests are failing! I'm not sure the details of why this started now, but `StopIteration` was being raised in a generator which should just return instead.

paleolimbot

Thank you!

(I should have merged this before the pre-commit one...I'll take care of that 😬 )

Follow on to #258. Closes #264.

WillAyd added 3 commits July 17, 2023 11:20

file updates

2cc7772

Working test suite

bb1e708

clang-format

8fd3664

lidavidm reviewed Jul 17, 2023

View reviewed changes

replace void pointer with union

eb83b21

paleolimbot reviewed Jul 17, 2023

View reviewed changes

feedback

963ebaa

fixed memory leak in tests

f998301

WillAyd added 3 commits July 18, 2023 09:05

Flattened struct

0dc7c08

Removed ArrowIntervalUnit

f415210

sizeof macro

9e37cc3

paleolimbot reviewed Jul 19, 2023

View reviewed changes

paleolimbot mentioned this pull request Jul 19, 2023

fix(python): Ensure generator does not raise StopIteration #262

Merged

WillAyd added 3 commits July 19, 2023 10:44

Merge branch 'main' into add-interval

92ada71

feedback

a6bebd4

reverted dist

c1a2c56

paleolimbot approved these changes Jul 20, 2023

View reviewed changes

paleolimbot merged commit 9e5a991 into apache:main Jul 20, 2023
26 checks passed

paleolimbot mentioned this pull request Jul 20, 2023

Add ArrowArrayViewGetInterval() #264

Closed

WillAyd mentioned this pull request Jul 28, 2023

Add ArrowArrayViewGetIntervalUnsafe #270

Merged

paleolimbot pushed a commit that referenced this pull request Aug 1, 2023

Add ArrowArrayViewGetIntervalUnsafe (#270)

5c3e149

Follow on to #258. Closes #264.

paleolimbot modified the milestone: nanoarrow 0.4.0 Sep 21, 2023

paleolimbot added this to the nanoarrow 0.3.0 milestone Sep 21, 2023

WillAyd deleted the add-interval branch September 5, 2024 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Intervals #258

Add Support for Intervals #258

WillAyd commented Jul 17, 2023

lidavidm Jul 17, 2023

codecov-commenter commented Jul 17, 2023 •

edited

Loading

paleolimbot left a comment

paleolimbot Jul 17, 2023

WillAyd Jul 17, 2023 •

edited

Loading

paleolimbot Jul 18, 2023

WillAyd Jul 18, 2023

WillAyd Jul 18, 2023

paleolimbot Jul 18, 2023

paleolimbot Jul 18, 2023

WillAyd Jul 19, 2023

WillAyd commented Jul 18, 2023

WillAyd commented Jul 18, 2023

paleolimbot commented Jul 18, 2023

paleolimbot commented Jul 18, 2023

WillAyd commented Jul 19, 2023

paleolimbot left a comment

paleolimbot Jul 19, 2023

paleolimbot Jul 19, 2023

paleolimbot Jul 19, 2023

paleolimbot left a comment

Add Support for Intervals #258

Add Support for Intervals #258

Conversation

WillAyd commented Jul 17, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jul 17, 2023 • edited Loading

Codecov Report

paleolimbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Jul 18, 2023

WillAyd commented Jul 18, 2023

paleolimbot commented Jul 18, 2023

paleolimbot commented Jul 18, 2023

WillAyd commented Jul 19, 2023

paleolimbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paleolimbot left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 17, 2023 •

edited

Loading

WillAyd Jul 17, 2023 •

edited

Loading