test: construct temporal values from structural inputs #267
Conversation
| #include "iceberg/util/temporal_util.h" | ||
|
|
||
| #include <chrono> | ||
| #include <cstdint> |
There was a problem hiding this comment.
has already been added by the header
There was a problem hiding this comment.
This hasn't been directly added by the temporal_util.h, there are uses of int32_t in this file, so I think include the header is fine?
src/iceberg/util/temporal_util.h
Outdated
| static Result<Literal> ExtractHour(const Literal& literal); | ||
|
|
||
| /// \brief Construct a Calendar date without timezone or time | ||
| static int32_t CreateDate(const TemporalParts& parts); |
There was a problem hiding this comment.
Should these functions return a Result<>, returning an error message when the parameters in TemporalParts are invalid?
There was a problem hiding this comment.
I just moved these constructors into the test directory, not user-facing API any more, so I think returning the value directly should be fine.
src/iceberg/util/temporal_util.h
Outdated
|
|
||
| namespace iceberg { | ||
|
|
||
| struct ICEBERG_EXPORT TemporalParts { |
There was a problem hiding this comment.
I was thinking that only adding these structures to the test directory to make it easy for us to create Date, Time and Timestamp values in the test cases. Temporal values are notorious to deal with, especially when it comes to timezone and daylight saving rules. We don't want to expose such APIs externally and maintain it hereafter.
There was a problem hiding this comment.
Perhaps we should not reuse a single structure to create different temporal types, which leads to unused fields.
wgtmac
changed the title
3 participants
No description provided.