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

Add support for pretty-printing Decimal numbers #230

Closed
alamb opened this issue Apr 27, 2021 · 3 comments · Fixed by #273
Closed

Add support for pretty-printing Decimal numbers #230

alamb opened this issue Apr 27, 2021 · 3 comments · Fixed by #273
Assignees
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog good first issue Good for newcomers

Comments

@alamb
Copy link
Contributor

alamb commented Apr 27, 2021

In the context of apache/datafusion#107 from @joshuataylor

Describe the solution you'd like
It should be possible to pretty-print columns of DataType::Decimal type. Pretty printing is used for datafusion and other projects to make human readable versions of Arrow arrays.

To do so, we would add support here:

_ => Err(ArrowError::InvalidArgumentError(format!(
"Pretty printing not implemented for {:?} type",
column.data_type()

Right now, you get an error such as

ArrowError(InvalidArgumentError("Pretty printing not implemented for Decimal(9, 0) type"))
@alamb alamb added good first issue Good for newcomers arrow Changes to the arrow crate labels Apr 27, 2021
@mgill25
Copy link
Contributor

mgill25 commented May 2, 2021

Hi @alamb I'd like to take this issue up if that's ok.

Looks like the change should be simple enough - just add the Decimal pattern to the match expression. I have a silly question however: Is this being tested somewhere?

Seems like there is no test in display.rs, but there are tests in pretty.rs. Which I tried to run these tests, it seems cargo is skipping them, and yet it doesn't skip the tests in other neighboring modules.

Screenshot 2021-05-02 at 21 14 50

Am I overthinking this a bit? Let me know how to figure this one out. :) Thanks!

Reason for tests: I'd like to see the change in action, so to speak.

@nevi-me
Copy link
Contributor

nevi-me commented May 2, 2021

@mgill25 the pretty tests are likely skipped because prettyprintis an optional feature

@alamb
Copy link
Contributor Author

alamb commented May 3, 2021

Thanks @mgill25 ! As @nevi-me says the pretty print feature is behind a feature flag (as it baloons the dependency stack with something that does not support for WASM builds, as I recall)

To answer your question, the pretty print feature is tested here in CI:
https://github.com/apache/arrow-rs/blob/master/.github/workflows/rust.yml#L114

To test it locally, you can run cargo like:

          cargo test --features=prettyprint

mgill25 pushed a commit to mgill25/arrow-rs that referenced this issue May 9, 2021
alamb pushed a commit that referenced this issue May 14, 2021
* Added Decimal support to pretty-print display utility (#230)

* Applied cargo fmt to fix linting errors

* Added proper printing for decimals based on scale, moved tests to pretty.rs

* Applied cargo fmt on pretty test

Co-authored-by: Manish Gill <manish.gill@tomtom.com>
@alamb alamb added the enhancement Any new improvement worthy of a entry in the changelog label May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants