Skip to content
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

Support for Decimal128 & Decimal256 without downcasting to f64 #305

Open
Jeadie opened this issue Apr 30, 2024 · 6 comments
Open

Support for Decimal128 & Decimal256 without downcasting to f64 #305

Jeadie opened this issue Apr 30, 2024 · 6 comments

Comments

@Jeadie
Copy link
Contributor

Jeadie commented Apr 30, 2024

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?

@Maxxen
Copy link
Member

Maxxen commented May 2, 2024

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.

@Mause
Copy link
Member

Mause commented May 2, 2024

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

@Jeadie
Copy link
Contributor Author

Jeadie commented May 3, 2024

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.

@Mause
Copy link
Member

Mause commented May 3, 2024

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

@Jeadie
Copy link
Contributor Author

Jeadie commented May 3, 2024

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:

@Maxxen
Copy link
Member

Maxxen commented Jun 6, 2024

As mentioned DuckDB doesn't support decimal256 or decimals with negative scale, but I've added support for positive decimal128 conversion here #328

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants