Implement Type and Array for Decimal32 and Decimal64 #7098

CurtHagenlocher · 2025-02-08T02:36:09Z

Which issue does this PR close?

Part of addressing #6661 but does not close it.

What changes are included in this PR?

This change implements Decimal32Array, Decimal32Type, Decimal64Array and Decimal64Type in a relatively minimal fashion. It doesn't implement the full range of casts or support for Parquet or CSV reader. These will come in the next change.

Are there any user-facing changes?

Adds several public types. Adds new entries for the DataType enum, which is a breaking change.

Jefffrey

Changes make sense to me (though is a bit harder to identify where things might not have been added like tests, etc.)

However, I think the addition of new values to DataType enum would make this a breaking change so would need to wait for next major release?

Jefffrey · 2025-02-08T10:22:03Z

arrow-schema/src/datatype.rs

+
+/// The default scale for [DataType::Decimal64] values
+pub const DECIMAL64_DEFAULT_SCALE: i8 = 6;
+


Do we have a reference for these constants (both max precision/scale & defaults)?

Ah, I'd picked the default scales mostly arbitrarily. The DecimalType contract requires one, and there isn't really any obviously-correct choice. PostgreSQL has a money type which is roughly DECIMAL(~18, 2). MSSQL has a smallmoney that's ~9, 4 and a money that's ~18, 4. Four seems a little big for a 32-bit decimal and a little small for a 64-bit decimal. I'd love to get feedback about these, but also don't know how important they are.

As for precision, it's based on the maximum number of decimal digits that can be stored without running into the space limits, just like the existing decimals. The largest i32 is 2,147,483,647 and that can reliably store 999,999,999 but not 9,999,999,999 -- so the max precision is 9.

Perhaps the default scales should get wider discussion? Where would be the right place for that?

Makes sense for precision. For default scale, do we have a reference from the C++ implementation possibly, to align with that?

I haven't found defaults in either the C++ or the Go implementation, and I know there's none in the C# code.

Jefffrey · 2025-02-08T10:22:27Z

parquet/src/arrow/arrow_reader/statistics.rs

+                        ParquetStatistics::Int64(s) => s
+                            .$func()
+                            .map(|x| $stat_value_type::try_from(*x).ok())
+                            .flatten(),


Are these changes significant?

Both of these changes are required for the code to compile, so I'm somewhat leaning on the compiler here. My naive take was that there shouldn't be a performance loss from the change (and that this wouldn't be a performance-critical path anyway) but I'm willing to believe that I might be wrong.

I think it's fine, was just curious as these changes specifically didn't seem related to the rest here, so wondering what else was changed that made the compiler require this change.

Jefffrey · 2025-02-08T10:22:48Z

parquet/src/arrow/arrow_reader/statistics.rs

@@ -730,7 +783,7 @@ macro_rules! get_decimal_page_stats_iterator {
                            native_index
                                .indexes
                                .iter()
-                                .map(|x| x.$func.and_then(|x| Some($stat_value_type::from(x))))
+                                .map(|x| x.$func.and_then(|x| $stat_value_type::try_from(x).ok()))


CurtHagenlocher · 2025-02-08T13:30:47Z

Changes make sense to me (though is a bit harder to identify where things might not have been added like tests, etc.)

I'd gone through and searched the codebase for all instances of Decimal128 and Decimal256 and amended nearly all of them. For this subset of the changes, I think I got most of the non-benchmark tests. I was thinking there would be two more PRs, one to cover casts, changes to Parquet and CSV reader/writers, and benchmarks, and then one that does a final pass through the code to account for things that I originally missed due to the volume of changes and things that were added after I originally did this work in the end of December.

However, I think the addition of new values to DataType enum would make this a breaking change so would need to wait for next major release?

Yes, it's definitely a breaking change. I have to admit that I'm not familiar with the arrow-rs release schedule. I'm used to the main branch of the primary Arrow repo which is always "Vnext". How does that work here?

alamb · 2025-02-08T16:31:04Z

However, I think the addition of new values to DataType enum would make this a breaking change so would need to wait for next major release?

Yes I think that is the case

Per the schedule this means we could merge this PR once we have branched for 54.2 (early march) https://github.com/apache/arrow-rs?tab=readme-ov-file#arrow-and-parquet-crates

CurtHagenlocher · 2025-02-08T16:37:34Z

Per the schedule this means we could merge this PR once we have branched for 54.2 (early march) https://github.com/apache/arrow-rs?tab=readme-ov-file#arrow-and-parquet-crates

Got it. In light of this, I'll likely withdraw the PR for now or move to Draft status. It would be good but not critical to figure out what we want as the default scale for the new types.

Jefffrey

We can leave the PR up; I've tagged with the next-major-release tag for visibility

Jefffrey · 2025-02-09T01:11:16Z

parquet/src/arrow/arrow_reader/statistics.rs

+                        ParquetStatistics::Int64(s) => s
+                            .$func()
+                            .map(|x| $stat_value_type::try_from(*x).ok())
+                            .flatten(),


I think it's fine, was just curious as these changes specifically didn't seem related to the rest here, so wondering what else was changed that made the compiler require this change.

Jefffrey · 2025-02-09T01:11:58Z

arrow-schema/src/datatype.rs

+
+/// The default scale for [DataType::Decimal64] values
+pub const DECIMAL64_DEFAULT_SCALE: i8 = 6;
+


Makes sense for precision. For default scale, do we have a reference from the C++ implementation possibly, to align with that?

Implement Type and Array for Decimal32 and Decimal64

62fa67d

github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Feb 8, 2025

Fix formatting

56d75c3

Jefffrey reviewed Feb 8, 2025

View reviewed changes

Jefffrey added the next-major-release the PR has API changes and it waiting on the next major version label Feb 9, 2025

Jefffrey reviewed Feb 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Type and Array for Decimal32 and Decimal64 #7098

Implement Type and Array for Decimal32 and Decimal64 #7098

CurtHagenlocher commented Feb 8, 2025 •

edited

Loading

Jefffrey left a comment

Jefffrey Feb 8, 2025

CurtHagenlocher Feb 8, 2025 •

edited

Loading

CurtHagenlocher Feb 8, 2025

Jefffrey Feb 9, 2025

CurtHagenlocher Feb 9, 2025

Jefffrey Feb 8, 2025

CurtHagenlocher Feb 8, 2025

Jefffrey Feb 9, 2025

Jefffrey Feb 8, 2025

CurtHagenlocher commented Feb 8, 2025

alamb commented Feb 8, 2025

CurtHagenlocher commented Feb 8, 2025

Jefffrey left a comment

Jefffrey Feb 9, 2025

Jefffrey Feb 9, 2025


		/// The default scale for [DataType::Decimal64] values
		pub const DECIMAL64_DEFAULT_SCALE: i8 = 6;

Implement Type and Array for Decimal32 and Decimal64 #7098

Are you sure you want to change the base?

Implement Type and Array for Decimal32 and Decimal64 #7098

Conversation

CurtHagenlocher commented Feb 8, 2025 • edited Loading

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

Jefffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CurtHagenlocher Feb 8, 2025 • 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

Choose a reason for hiding this comment

CurtHagenlocher commented Feb 8, 2025

alamb commented Feb 8, 2025

CurtHagenlocher commented Feb 8, 2025

Jefffrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CurtHagenlocher commented Feb 8, 2025 •

edited

Loading

CurtHagenlocher Feb 8, 2025 •

edited

Loading