-
Notifications
You must be signed in to change notification settings - Fork 839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add MONTH_DAY_NANO interval type, impl ArrowNativeType
for i128
#779
Changes from all commits
8c7e4e5
4791a97
4113256
3c2155e
1ff27a3
a608be5
2ad60f1
4e19eb8
cb5a237
b2065ad
1304df6
74abfcb
49d503e
3571b1b
adc36f1
cf8402d
b628d49
841321a
cb1c363
3947f04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -444,6 +444,7 @@ def_numeric_from_vec!(Time64MicrosecondType); | |
def_numeric_from_vec!(Time64NanosecondType); | ||
def_numeric_from_vec!(IntervalYearMonthType); | ||
def_numeric_from_vec!(IntervalDayTimeType); | ||
def_numeric_from_vec!(IntervalMonthDayNanoType); | ||
def_numeric_from_vec!(DurationSecondType); | ||
def_numeric_from_vec!(DurationMillisecondType); | ||
def_numeric_from_vec!(DurationMicrosecondType); | ||
|
@@ -649,6 +650,23 @@ mod tests { | |
assert!(arr.is_null(1)); | ||
assert_eq!(-5, arr.value(2)); | ||
assert_eq!(-5, arr.values()[2]); | ||
|
||
// a month_day_nano interval contains months, days and nanoseconds, | ||
// but we do not yet have accessors for the values. | ||
// TODO: implement month, day, and nanos access method for month_day_nano. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 When this PR is merged, I will try and file a ticket for adding these accessors -- I think it would be a fairly good "first PR" type change for new contributors |
||
let arr = IntervalMonthDayNanoArray::from(vec![ | ||
Some(100000000000000000000), | ||
None, | ||
Some(-500000000000000000000), | ||
]); | ||
assert_eq!(3, arr.len()); | ||
assert_eq!(0, arr.offset()); | ||
assert_eq!(1, arr.null_count()); | ||
assert_eq!(100000000000000000000, arr.value(0)); | ||
assert_eq!(100000000000000000000, arr.values()[0]); | ||
assert!(arr.is_null(1)); | ||
assert_eq!(-500000000000000000000, arr.value(2)); | ||
assert_eq!(-500000000000000000000, arr.values()[2]); | ||
} | ||
|
||
#[test] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -243,6 +243,11 @@ pub fn sort_to_indices( | |
DataType::Interval(IntervalUnit::DayTime) => { | ||
sort_primitive::<IntervalDayTimeType, _>(values, v, n, cmp, &options, limit) | ||
} | ||
DataType::Interval(IntervalUnit::MonthDayNano) => { | ||
sort_primitive::<IntervalMonthDayNanoType, _>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The i128 order relationship does not hold for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can I write a function like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that in general the time intervals do not have an natural order without an associated datetime pinning them to a specific time line. The conversion |
||
values, v, n, cmp, &options, limit, | ||
) | ||
} | ||
DataType::Duration(TimeUnit::Second) => { | ||
sort_primitive::<DurationSecondType, _>(values, v, n, cmp, &options, limit) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is often much concern about adding new dependencies to arrow - however, this feature does not seem to add any new dependencies: https://github.com/serde-rs/json/blob/master/Cargo.toml#L74
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this feature because we need to deserialize i128 numbers
https://github.com/serde-rs/json/blob/master/src/number.rs#L534