[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #258: Add Support for Intervals

["paleolimbot (via GitHub)" <gi...@apache.org>]

paleolimbot commented on code in PR #258:
URL: https://github.com/apache/arrow-nanoarrow/pull/258#discussion_r1265840547


##########
src/nanoarrow/nanoarrow.h:
##########
@@ -870,6 +870,14 @@ static inline ArrowErrorCode ArrowArrayAppendBytes(struct ArrowArray* array,
 static inline ArrowErrorCode ArrowArrayAppendString(struct ArrowArray* array,
                                                     struct ArrowStringView value);
 
+/// \brief Append a Interval to an array
+///
+/// Returns NANOARROW_OK if value can be exactly represented by
+/// the underlying storage type or EINVAL otherwise (e.g., value
+/// is outside the valid array range).

Review Comment:
   This probably doesn't apply to the interval?



##########
src/nanoarrow/nanoarrow_types.h:
##########
@@ -656,6 +672,19 @@ struct ArrowArrayPrivateData {
   int8_t union_type_id_is_child_index;
 };
 
+/// \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;
+};

Review Comment:
   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?



##########
src/nanoarrow/array_test.cc:
##########
@@ -29,6 +29,7 @@
 #include <arrow/util/decimal.h>
 
 #include "nanoarrow/nanoarrow.h"
+#include "nanoarrow/nanoarrow_types.h"

Review Comment:
   Is this include needed?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org