feat: Spark-4.0 widening type support #604

kazuyukitanimura · 2024-06-28T07:19:09Z

Which issue does this PR close?

Part of #372 and #551

Rationale for this change

To be ready for Spark 4.0

What changes are included in this PR?

This PR adds type widening feature support introduced in Spark-4.0

How are these changes tested?

Enabled ParquetTypeWideningSuite

core/src/parquet/read/column.rs

parthchandra

lgtm

kazuyukitanimura · 2024-07-12T23:13:11Z

This is ready for review @andygrove @comphead @huaxingao @viirya
@parthchandra already approved it.

andygrove · 2024-07-16T13:39:27Z

native/core/src/parquet/read/values.rs

+                if dst_scale > src_scale {
+                    let mul = (10 as $dst_type).pow(dst_scale - src_scale);
+                    for i in 0..num {
+                        unsafe {


Could you add SAFETY comments for the unsafe blocks that explain how/why they are actually safe?

andygrove

I'm not an expert on this area but the changes seem reasonable to me. I think it would be good practice for us to document any unsafe code that we are adding.

andygrove · 2024-07-16T13:43:16Z

native/core/src/parquet/read/values.rs

+        {
+            let mut offset = src.offset;
+            for _ in 0..num {
+                let v = &src_data[offset..offset + byte_width] as *const [u8] as *const u8


Is there a reason why we have to use as *const [u8] as *const u8 as *const i32 rather than .as_ptr() as *const i32?

parthchandra · 2024-07-17T18:26:13Z

btw, this is really a feat not a fix :)

comphead · 2024-07-17T18:50:38Z

native/core/benches/parquet_read.rs

@@ -54,7 +54,7 @@ fn bench(c: &mut Criterion) {
        );
        b.iter(|| {
            let cd = ColumnDescriptor::new(t.clone(), 0, 0, ColumnPath::from(Vec::new()));
-            let promition_info = TypePromotionInfo::new(PhysicalType::INT32, -1, -1);
+            let promition_info = TypePromotionInfo::new(PhysicalType::INT32, -1, -1, 32);


Suggested change

let promition_info = TypePromotionInfo::new(PhysicalType::INT32, -1, -1, 32);

let promotion_info = TypePromotionInfo::new(PhysicalType::INT32, -1, -1, 32);

comphead · 2024-07-17T18:51:32Z

native/core/src/parquet/data_type.rs

@@ -48,6 +52,7 @@ make_type!(FLBADecimal32Type);
 make_type!(FLBADecimal64Type);
 make_type!(FLBAType);
 make_type!(Int32DateType);
+make_type!(Int32TimestampMicrosType);


should we have Int32TimestampMillisType as well?

We don't have a usage for mills for now I think

comphead · 2024-07-17T18:52:40Z

native/core/src/parquet/read/values.rs

+make_int_variant_dict_impl!(Int32To64Type, i32, i64);
+make_int_variant_dict_impl!(Int32ToDecimal64Type, i32, i64);
+make_int_variant_dict_impl!(Int32ToDoubleType, i32, f64);
+make_int_variant_dict_impl!(Int32TimestampMicrosType, i32, i64);


Int32 Millis?

We don't have a usage for mills for now I think

comphead · 2024-07-17T18:53:59Z

native/core/src/parquet/read/values.rs

+
+                // TODO: optimize this further as checking value one by one is not very efficient
+                unsafe {
+                    if unlikely(v.read_unaligned() < JULIAN_GREGORIAN_SWITCH_OFF_DAY) {


should we use unlikely as its rust nightly feature?

We have our own unlikely https://github.com/apache/datafusion-comet/blob/main/native/core/src/lib.rs#L126

comphead · 2024-07-17T18:54:40Z

native/core/src/parquet/read/values.rs

+                    if unlikely(v.read_unaligned() < JULIAN_GREGORIAN_SWITCH_OFF_DAY) {
+                        panic!(
+                            "Encountered timestamp value {}, which is before 1582-10-15 (counting \
+                        backwards from Unix eopch date 1970-01-01), and could be ambigous \


Suggested change

backwards from Unix eopch date 1970-01-01), and could be ambigous \

backwards from Unix epoch date 1970-01-01), and could be ambiguous \

comphead · 2024-07-17T18:57:48Z

native/core/src/parquet/read/values.rs

+generate_cast_to_signed!(copy_f32_to_f64, f32, f64);
+
+fn copy_i64_to_i64(src: &[u8], dst: &mut [u8], num: usize) {
+    debug_assert!(


wondering can it be a single check?

This is debug_assert! so should be fine to check every time

comphead · 2024-07-17T18:58:05Z

native/core/src/parquet/read/values.rs

+        "Destination slice is too small"
+    );
+
+    bit::memcpy_value(src, 8 * num, dst);


8 here is std::mem::size_of:: ?

comphead

Thanks @kazuyukitanimura I'm wondering should be have tests to read those types?

kazuyukitanimura · 2024-07-17T19:15:58Z

btw, this is really a feat not a fix :)

updated

kazuyukitanimura · 2024-07-17T19:18:19Z

Thanks @kazuyukitanimura I'm wondering should be have tests to read those types?

Thanks @comphead
Spark's ParquetTypeWideningSuite has a good coverage

kazuyukitanimura · 2024-07-17T19:21:00Z

@parthchandra @andygrove
I have added a relevant change to fix one more test after your approvals 3659f0a
Do you mind taking another look?

native/core/src/parquet/read/levels.rs

parthchandra

LGTM. Except one comment (mostly for my understanding)

kazuyukitanimura · 2024-07-18T19:39:23Z

Merged, thank you @parthchandra @andygrove @comphead

## Rationale for this change To be ready for Spark 4.0 ## What changes are included in this PR? This PR adds type widening feature support introduced in Spark-4.0 ## How are these changes tested? Enabled ParquetTypeWideningSuite

fix: Spark-4.0 widening type support

7329648

parthchandra reviewed Jul 2, 2024

View reviewed changes

core/src/parquet/read/column.rs Outdated Show resolved Hide resolved

kazuyukitanimura added 3 commits July 9, 2024 17:02

Merge remote-tracking branch 'upstream/main' into spark-4.0-widening

a65b38b

fix: Spark-4.0 widening type support

003420c

Merge remote-tracking branch 'upstream/main' into spark-4.0-widening

d12b9b3

parthchandra approved these changes Jul 12, 2024

View reviewed changes

kazuyukitanimura added 3 commits July 12, 2024 13:27

fix: Spark-4.0 widening type support

30d448b

fix: Spark-4.0 widening type support

038a8b3

fix: Spark-4.0 widening type support

87faf1f

kazuyukitanimura marked this pull request as ready for review July 12, 2024 23:11

kazuyukitanimura added 2 commits July 15, 2024 12:56

fix: Spark-4.0 widening type support

0232334

fix: Spark-4.0 widening type support

36c3cce

andygrove reviewed Jul 16, 2024

View reviewed changes

andygrove approved these changes Jul 16, 2024

View reviewed changes

andygrove reviewed Jul 16, 2024

View reviewed changes

kazuyukitanimura added 2 commits July 16, 2024 19:17

fix: Spark-4.0 widening type support

3659f0a

Merge remote-tracking branch 'upstream/main' into spark-4.0-widening

1a37f2a

comphead reviewed Jul 17, 2024

View reviewed changes

address review comments

d1a6011

kazuyukitanimura changed the title ~~fix: Spark-4.0 widening type support~~ feat: Spark-4.0 widening type support Jul 17, 2024

parthchandra reviewed Jul 17, 2024

View reviewed changes

native/core/src/parquet/read/levels.rs Show resolved Hide resolved

parthchandra approved these changes Jul 17, 2024

View reviewed changes

kazuyukitanimura merged commit 64b5f3c into apache:main Jul 18, 2024
75 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Spark-4.0 widening type support #604

feat: Spark-4.0 widening type support #604

kazuyukitanimura commented Jun 28, 2024

parthchandra left a comment

kazuyukitanimura commented Jul 12, 2024

andygrove Jul 16, 2024

kazuyukitanimura Jul 17, 2024

andygrove left a comment

andygrove Jul 16, 2024

kazuyukitanimura Jul 17, 2024

parthchandra commented Jul 17, 2024

comphead Jul 17, 2024

kazuyukitanimura Jul 17, 2024

comphead Jul 17, 2024

kazuyukitanimura Jul 17, 2024

comphead Jul 17, 2024

kazuyukitanimura Jul 17, 2024

comphead Jul 17, 2024

kazuyukitanimura Jul 17, 2024

comphead Jul 17, 2024

kazuyukitanimura Jul 17, 2024

comphead Jul 17, 2024

kazuyukitanimura Jul 17, 2024

comphead Jul 17, 2024

kazuyukitanimura Jul 17, 2024

comphead left a comment

kazuyukitanimura commented Jul 17, 2024

kazuyukitanimura commented Jul 17, 2024

kazuyukitanimura commented Jul 17, 2024

parthchandra left a comment

kazuyukitanimura commented Jul 18, 2024

	let promition_info = TypePromotionInfo::new(PhysicalType::INT32, -1, -1, 32);
	let promotion_info = TypePromotionInfo::new(PhysicalType::INT32, -1, -1, 32);

	backwards from Unix eopch date 1970-01-01), and could be ambigous \
	backwards from Unix epoch date 1970-01-01), and could be ambiguous \

feat: Spark-4.0 widening type support #604

feat: Spark-4.0 widening type support #604

Conversation

kazuyukitanimura commented Jun 28, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

parthchandra left a comment

Choose a reason for hiding this comment

kazuyukitanimura commented Jul 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthchandra commented Jul 17, 2024

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

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

comphead left a comment

Choose a reason for hiding this comment

kazuyukitanimura commented Jul 17, 2024

kazuyukitanimura commented Jul 17, 2024

kazuyukitanimura commented Jul 17, 2024

parthchandra left a comment

Choose a reason for hiding this comment

kazuyukitanimura commented Jul 18, 2024