duckdb-rs icon indicating copy to clipboard operation
duckdb-rs copied to clipboard

Support for Decimal128 & Decimal256 without downcasting to f64

Open Jeadie opened this issue 1 year ago • 7 comments

Overview

Currently Duckdb-rs supports Decimal128, but not Decimal256. However, its support for Decimal128 downcasts to f64. See decimal_array_to_vector.

/// Convert Arrow [Decimal128Array] to a duckdb vector.
fn decimal_array_to_vector(array: &Decimal128Array, out: &mut FlatVector) {
    assert!(array.len() <= out.capacity());
    for i in 0..array.len() {
        out.as_mut_slice()[i] = array.value_as_string(i).parse::<f64>().unwrap();
    }
}

This fails a check_rust_primitive_array_roundtrip, when I ran it locally.

assertion `left == right` failed
  left: Float64
 right: Decimal128(38, 10)

The underlying cause of the issue is that duckdb-rs explicitly converts to Double, and not Decimal see code

       // duckdb/src/main/capi/helper-c.cpp does not support decimal
       // DataType::Decimal128(_, _) => Decimal,
       // DataType::Decimal256(_, _) => Decimal,
       DataType::Decimal128(_, _) => Double,
       DataType::Decimal256(_, _) => Double,

The comment above reference the fact Decimal does not appear in https://github.com/duckdb/duckdb/blob/main/src/main/capi/helper-c.cpp#L5-L61.

Feature

Support for Decimal128 and Decimal256.

Question

Is this issue in capi/helper-c.cpp an intrinisic issue with DuckDB, and if not, what work would be required to support all Decimal types in the Rust bindings?

Jeadie avatar Apr 30 '24 22:04 Jeadie

Hi! I don't know if helper-c.cpp matters, struct, union and list types aren't present there either, ~~but It's true that the c-api doesn't expose a way to create decimal types using the standard duckdb_create_logical_type. The issue is that since decimal types are "parameterized" you'd need a different constructor function that allows you to pass the width and scale, (similar to e.g. duckdb_create_list_type), but thats definitely something we could look into adding.~~

We do have a duckdb_create_decimal_type and a duckdb_create_decimal function, so adding support for this in the rust api not be that complex.

In the case of 256-width decimals however, thats more of an expected limitation as DuckDB itself doesn't support 256-width decimals internally.

Maxxen avatar May 02 '24 09:05 Maxxen

The vtab module does have some support already: https://docs.rs/duckdb/latest/duckdb/vtab/struct.LogicalType.html#method.decimal

Mause avatar May 02 '24 09:05 Mause

Even if DuckDB-rs has some typing for Decimal128, I think the core issue is that duckdb_execute_prepared_arrow (i.e. https://github.com/duckdb/duckdb/blob/5b36f520df72be12f90ae31e6f8ed797a7b78e99/src/main/capi/arrow-c.cpp#L163) uses helper-c.cpp and if that doesn't support Decimal128, no change to only duckdb-rs will fix it.

I don't have the best understanding of duckdb specifically.

Jeadie avatar May 03 '24 05:05 Jeadie

Where are you seeing that function used in the codepath duckdb-rs uses?

Mause avatar May 03 '24 05:05 Mause

Running

let schema = Schema::new(vec![Field::new("a", input_array.data_type().clone(), false)]);
let rb = RecordBatch::try_new(Arc::new(schema), vec![Arc::new(input_array.clone())])?;
let mut stmt = db.prepare("select a from arrow(?, ?)")?;
let rb = stmt.query_arrow(param)?

stmt.query_arrow calls duckdb_execute_prepared_arrow, via:

  • https://github.com/duckdb/duckdb-rs/blob/a1aa55aff22b75e149e9cf7cface6464b3dc0ccc/src/statement.rs#L107
  • https://github.com/duckdb/duckdb-rs/blob/a1aa55aff22b75e149e9cf7cface6464b3dc0ccc/src/statement.rs#L63
  • https://github.com/duckdb/duckdb-rs/blob/a1aa55aff22b75e149e9cf7cface6464b3dc0ccc/src/statement.rs#L510
  • https://github.com/duckdb/duckdb-rs/blob/a1aa55aff22b75e149e9cf7cface6464b3dc0ccc/src/raw_statement.rs#L223
  • https://github.com/duckdb/duckdb-rs/blob/a1aa55aff22b75e149e9cf7cface6464b3dc0ccc/src/raw_statement.rs#L227

Jeadie avatar May 03 '24 06:05 Jeadie

As mentioned DuckDB doesn't support decimal256 or decimals with negative scale, but I've added support for positive decimal128 conversion here https://github.com/duckdb/duckdb-rs/pull/328

Maxxen avatar Jun 06 '24 13:06 Maxxen