Structured interval types for `IntervalMonthDayNano` or `IntervalDayTime` (#3125) (#5654) #5769

tustvold · 2024-05-15T18:17:11Z

Which issue does this PR close?

Closes #3125
Closes #5654

Rationale for this change

The current interval implementation in rust is incorrect (the bit fields are wrong) as explained on #5654

Due to the fact that intervals are stored as i128 simply updating construction and access for reading the fields would have at least two unpleasant effects:

Downstream code that use intervals might silently break / start working very differently
Sorting would become somewhat nonsensical as

What changes are included in this PR?

Make IntervalMonthDayNanoand IntervalDayTime structured types
Update IntervalMonthDayNanoArray and IntervalDayTimeArray appropriately
Remove cast support from the Int64Arrays to/from those types

Are there any user-facing changes?

Yes, this is a major breaking change for anyone who uses IntervalMonthDayNano or IntervalDayTime

tustvold · 2024-05-15T18:18:11Z

arrow-buffer/src/interval.rs

+
+    /// Computes the absolute value
+    #[inline]
+    pub fn wrapping_abs(self) -> Self {


These arithmetic operations are perhaps a little odd, but it's necessary to implement them in order to keep the type machinery happy

tustvold · 2024-05-15T18:30:45Z

arrow-buffer/src/native.rs

+    }
+
+    fn as_usize(self) -> usize {
+        (self.months as usize) | ((self.days as usize) << 32)


This is a bit contrived, but tests make use of this logic so we need to at least partially support it

arrow-buffer/src/interval.rs

crepererum

I really like the structured types, thanks a lot.

crepererum · 2024-05-16T11:30:40Z

arrow-array/src/types.rs

-│          (64 bits)           │ (32 bits)   │  (32 bits)   │
-└──────────────────────────────┴─────────────┴──────────────┘
-  0                            63            95           127 bit offset
+┌───────────────┬─────────────┬─────────────────────────────┐


remind me: do we have intergration tests that would find this issue?

Yes, and they're now actually testing the logic rather than doing the mapping themselves

In particular, the changes to arrow-integration-test/src/lib.rs in this PR

alamb

Thank you @tustvold -- I went through this PR and it looks very nice to me -- while it is a breaking change given the existing implementation is wrong I think this will at least make sure downstream users understand that.

I also filled out the PR description so when people come here they have some idea of what the context was and some of the rationale behind the decisions. It would be good if you could double check to make sure I am not mis-representing.

Thank you @crepererum for the review

cc @berkaysynnada and @ozankabak and @avantgardnerio who may be interested

alamb · 2024-05-17T16:48:01Z

arrow-array/src/arithmetic.rs

+native_type_op!(IntervalDayTime, IntervalDayTime::ZERO, IntervalDayTime::ONE);
+native_type_op!(
+    IntervalMonthDayNano,
+    IntervalMonthDayNano::ZERO,


I wondered if this needed any documentation, but it appears the answer is it already has some nice documentation 👍

alamb · 2024-05-17T16:49:20Z

arrow-array/src/types.rs

-│          (64 bits)           │ (32 bits)   │  (32 bits)   │
-└──────────────────────────────┴─────────────┴──────────────┘
-  0                            63            95           127 bit offset
+┌───────────────┬─────────────┬─────────────────────────────┐


In particular, the changes to arrow-integration-test/src/lib.rs in this PR

alamb · 2024-05-17T16:55:15Z

arrow-cast/src/cast/mod.rs

@@ -275,11 +275,6 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool {
            DayTime => false,
            MonthDayNano => false,
        },
-        (Int64, Interval(to_type)) => match to_type {


This removes direct casting support for Int64Array to IntervalArray (which presumably was interpreting the integers incorrectly anyways)

What should someone do who uses that cast in their code today?

Something like

let cast_array = IntervalArray::from_iter( int64_array .values() .map(|v| IntervalDayTime::from_parts(v<<32, v&0xffffffff) .collect()

?

Or more optimally

let a: IntervalDayTimeArray = int64_array.unary(|x| IntervalDayTime::new((x << 32) as i32, x as i32));

Although the exact specifics will depend on if they're wanting to emulate the old (incorrect) byte order or the new 😅

This ambiguity is why I removed it

As long as there is some way to get the old behavior (if desired) I think removing the built in API seems reasonable

alamb · 2024-05-17T17:00:15Z

arrow-buffer/src/arith.rs

+// specific language governing permissions and limitations
+// under the License.
+
+macro_rules! derive_arith {


Could you possibly document this a bit to give future readers about its purpose ?

alamb · 2024-05-17T17:04:32Z

arrow-integration-test/src/lib.rs

@@ -387,6 +398,25 @@ pub fn array_from_json(
                    1 => b.append_value(match value {
                        Value::Number(n) => n.as_i64().unwrap(),
                        Value::String(s) => s.parse().expect("Unable to parse string as i64"),
+                        _ => panic!("Unable to parse {value:?} as number"),


The panics look suspicious to me, but the rest of the code follows the same pattern

Yeah, this code was originally written solely for tests, and so is quite panic happy

alamb · 2024-05-17T17:07:51Z

arrow-ord/src/ord.rs

-        assert_eq!(Ordering::Greater, cmp(3, 1));
-        assert_eq!(Ordering::Greater, cmp(3, 2));
+        assert_eq!(Ordering::Less, cmp(0, 0)); // v1 vs v3
+        assert_eq!(Ordering::Equal, cmp(0, 3)); // v1 vs v1


It took me a while to trace through the double layer of indirection here. Looks good

alamb · 2024-05-17T17:09:15Z

arrow-row/src/fixed.rs

@@ -163,6 +165,44 @@ impl FixedLengthEncoding for f64 {
    }
 }

+impl FixedLengthEncoding for IntervalDayTime {


This adds support for the new structured types in the row encoder 👍

ozankabak · 2024-05-20T10:37:32Z

This LGTM, but @berkaysynnada will have a better idea on the work necessary on the DF side

Ref apache/arrow-rs#5769

* deps: update datafusion to 39.0.0, pyo3 to 0.21, and object_store to 0.10.1 `datafusion-common` also depends on `pyo3`, so they need to be upgraded together. * feat: remove GetIndexField datafusion replaced Expr::GetIndexField with a FieldAccessor trait. Ref apache/datafusion#10568 Ref apache/datafusion#10769 * feat: update ScalarFunction The field `func_name` was changed to `func` as part of removing `ScalarFunctionDefinition` upstream. Ref apache/datafusion#10325 * feat: incorporate upstream array_slice fixes Fixes #670 * update ExectionPlan::children impl for DatasetExec Ref apache/datafusion#10543 * update value_interval_daytime Ref apache/arrow-rs#5769 * update regexp_replace and regexp_match Fixes #677 * add gil-refs feature to pyo3 This silences pyo3's deprecation warnings for its new Bounds api. It's the 1st step of the migration, and should be removed before merge. Ref https://pyo3.rs/v0.21.0/migration#from-020-to-021 * fix signature for octet_length Ref apache/datafusion#10726 * update signature for covar_samp AggregateUDF expressions now have a builder API design, which removes arguments like filter and order_by Ref apache/datafusion#10545 Ref apache/datafusion#10492 * convert covar_pop to expr_fn api Ref: https://github.com/apache/datafusion/pull/10418/files * convert median to expr_fn api Ref apache/datafusion#10644 * convert variance sample to UDF Ref apache/datafusion#10667 * convert first_value and last_value to UDFs Ref apache/datafusion#10648 * checkpointing with a few todos to fix remaining compile errors * impl PyExpr::python_value for IntervalDayTime and IntervalMonthDayNano * convert sum aggregate function to UDF * remove unnecessary clone on double reference * apply cargo fmt * remove duplicate allow-dead-code annotation * update tpch examples for new pyarrow interval Fixes #665 * marked q11 tpch example as expected fail Ref #730 * add default stride of None back to array_slice

github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels May 15, 2024

tustvold commented May 15, 2024

View reviewed changes

tustvold force-pushed the structured-interval branch 2 times, most recently from f5a5127 to 30ae103 Compare May 15, 2024 18:29

tustvold commented May 15, 2024

View reviewed changes

Structured interval type (apache#3125) (apache#5654)

416958a

tustvold force-pushed the structured-interval branch from 30ae103 to 416958a Compare May 15, 2024 18:31

Update integration-test

25bf96a

tustvold force-pushed the structured-interval branch from efdc38f to 25bf96a Compare May 15, 2024 19:03

Fix 32-bit build

3dce0f6

tustvold mentioned this pull request May 16, 2024

Flip interval field ordering (#5654) #5656

Closed

tustvold added the api-change Changes to the arrow API label May 16, 2024

crepererum reviewed May 16, 2024

View reviewed changes

arrow-buffer/src/interval.rs Show resolved Hide resolved

crepererum reviewed May 16, 2024

View reviewed changes

crepererum approved these changes May 16, 2024

View reviewed changes

This was referenced May 17, 2024

Structured Interval Native Type #3125

Closed

Rust Interval definition incorrect #5654

Closed

alamb changed the title ~~Structured interval type (#3125) (#5654)~~ Structured interval types for IntervalMonthDayNano or IntervalDayTime (#3125) (#5654) May 17, 2024

alamb approved these changes May 17, 2024

View reviewed changes

Review feedback

6cfd7cd

tustvold merged commit cf59b6c into apache:master May 20, 2024
27 checks passed

alamb mentioned this pull request May 25, 2024

feat: add substrait support for Interval types and literals apache/datafusion#10646

Merged

waynexia mentioned this pull request Jun 2, 2024

Release arrow-rs / parquet version 52.0.0 #5688

Closed

3 tasks

Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Jun 13, 2024

update value_interval_daytime

e1c4ad9

Ref apache/arrow-rs#5769

kawadakk mentioned this pull request Jun 19, 2024

MutableArrayData::new expects a large alignment for IntervalMonthDayNanoType arrays #5917

Open

alamb mentioned this pull request Jun 21, 2024

Expose IntervalMonthDayNano and IntervalDayTime and update docs #5928

Merged

wjones127 mentioned this pull request Jul 16, 2024

feat: upgrade DataFusion, Arrow, PyO3, ObjectStore lancedb/lance#2594

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structured interval types for `IntervalMonthDayNano` or `IntervalDayTime` (#3125) (#5654) #5769

Structured interval types for `IntervalMonthDayNano` or `IntervalDayTime` (#3125) (#5654) #5769

tustvold commented May 15, 2024 •

edited by alamb

Loading

tustvold May 15, 2024 •

edited

Loading

tustvold May 15, 2024

crepererum left a comment

crepererum May 16, 2024

tustvold May 16, 2024

alamb May 17, 2024

alamb left a comment

alamb May 17, 2024

alamb May 17, 2024

alamb May 17, 2024

tustvold May 20, 2024 •

edited

Loading

alamb May 20, 2024

alamb May 17, 2024

alamb May 17, 2024

tustvold May 20, 2024

alamb May 17, 2024

alamb May 17, 2024

ozankabak commented May 20, 2024

Structured interval types for IntervalMonthDayNano or IntervalDayTime (#3125) (#5654) #5769

Structured interval types for IntervalMonthDayNano or IntervalDayTime (#3125) (#5654) #5769

Conversation

tustvold commented May 15, 2024 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crepererum 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

alamb 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

tustvold May 20, 2024 • 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

ozankabak commented May 20, 2024

Structured interval types for `IntervalMonthDayNano` or `IntervalDayTime` (#3125) (#5654) #5769

Structured interval types for `IntervalMonthDayNano` or `IntervalDayTime` (#3125) (#5654) #5769

tustvold commented May 15, 2024 •

edited by alamb

Loading

tustvold May 15, 2024 •

edited

Loading

tustvold May 20, 2024 •

edited

Loading