Skip to content

Conversation

okhsunrog
Copy link

For correct serialization of decimal types into JSON we want them to be serialized as strings instead of numbers so they don't loose precision.

@github-actions github-actions bot added the arrow label Sep 26, 2025
@0x501D 0x501D requested review from askalt and novartole September 26, 2025 05:11
let options = FormatOptions::new().with_display_error(true);
let formatter = ArrayFormatter::try_new(array, &options)?;
(Box::new(RawArrayFormatter(formatter)) as _, array.nulls().cloned())
(Box::new(formatter) as _, array.nulls().cloned())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about to extend FormatOptions instead? This allows users to choose the format and keeps backward compatibility.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, could you take a look?

@askalt
Copy link

askalt commented Sep 26, 2025

Thank you for the patch!

We need to fix tests here:
https://github.com/tarantool/arrow-rs/blob/main/arrow-json/src/writer/mod.rs

Btw, does decimal deserializer properly works with input strings?

@okhsunrog
Copy link
Author

I use parse_decimal from arrow-cast/src/parse.rs and it works good

@askalt
Copy link

askalt commented Sep 26, 2025

I use parse_decimal from arrow-cast/src/parse.rs and it works good

Ok, it seems that deserializer also uses it https://github.com/tarantool/arrow-rs/blob/main/arrow-json/src/reader/decimal_array.rs#L57-L60, so it will be able to deserialize strings as decimals.

duration_format: DurationFormat,
/// If set to `true`, decimal values will be formatted as strings instead of numbers
/// This is useful for JSON output to preserve precision
decimal_as_string: bool,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to change it to enum of formats instead as it's done for others?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I refactored it to use an enum.

okhsunrog and others added 2 commits October 10, 2025 13:26
Co-authored-by: Artem Novikov <33791946+novartole@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants