alamb commented on code in PR #8829:
URL: https://github.com/apache/arrow-rs/pull/8829#discussion_r2524529377
##########
arrow-cast/src/display.rs:
##########
@@ -57,23 +57,23 @@ pub enum DurationFormat {
pub struct FormatOptions<'a> {
/// If set to `true` any formatting errors will be written to the output
/// instead of being converted into a [`std::fmt::Error`]
- safe: bool,
+ pub safe: bool,
Review Comment:
> Make fields in FormatOptions public. This is necessary as the custom
ArrayFormatter must also have access to the formatting options. (see <NULL> in
the test)
Rather than making these public, what about adding accessors for them? I
think that would make it easier to change the underlying implementation in the
future without causing breaking API changes
I think by making all these fields `pub` it means people can construct
format options explicitly like
```rust
let options = FormatOptions {
safe: false,
null: "",
...
};
```
So adding any new field to the struct will be a breaking API change
If we keep them private fields, then we can add new fields without breaking
existing peopel
##########
arrow-cast/src/pretty.rs:
##########
@@ -131,7 +214,27 @@ pub fn pretty_format_batches_with_options(
results: &[RecordBatch],
options: &FormatOptions,
) -> Result<impl Display + use<>, ArrowError> {
- create_table(None, results, options)
+ create_table(None, results, options, None)
+}
+
+/// Create a visual representation of [`RecordBatch`]es with formatting
options.
+///
+/// # Arguments
+/// * `results` - A slice of record batches to display
+/// * `options` - [`FormatOptions`] that control the resulting display
+/// * `formatters` - A slice of [`ArrayFormatter`]s that control the
formatting of each column. If
+/// a formatter is [`None`], the default formatter will be used. Must be
exactly as long as the
+/// number of fields in the record batches.
+///
+/// # Example
+///
+/// For an example, see [`ArrayFormatterFactory`].
+pub fn pretty_format_batches_with_options_and_formatters(
+ results: &[RecordBatch],
+ options: &FormatOptions,
+ formatters: Option<&dyn ArrayFormatterFactory>,
Review Comment:
Rather than needing a parameter, did you consider adding a new field to
`FormatOptions`?
```rust
struct FormatOptions {
...
formatter_factory: Option<&dyn ArrayFormatterFactory>,
}
```
That would reduce the new APIs and make it easier for existing code to use
custom formatters (just need to update options)
##########
arrow-cast/src/pretty.rs:
##########
@@ -22,15 +22,98 @@
//! [`RecordBatch`]: arrow_array::RecordBatch
//! [`Array`]: arrow_array::Array
-use std::fmt::Display;
-
use comfy_table::{Cell, Table};
+use std::fmt::{Display};
use arrow_array::{Array, ArrayRef, RecordBatch};
-use arrow_schema::{ArrowError, SchemaRef};
+use arrow_schema::{ArrowError, Field, SchemaRef};
use crate::display::{ArrayFormatter, FormatOptions};
+/// Allows creating a new [`ArrayFormatter`] for a given [`Array`] and an
optional [`Field`].
+///
+/// # Example
+///
+/// The example below shows how to create a custom formatter for a custom type
`my_money`.
+///
+/// ```rust
+/// use std::fmt::Write;
+/// use arrow_array::{Array, Int32Array, cast::AsArray};
+/// use arrow_cast::display::{ArrayFormatter, DisplayIndex, FormatOptions,
FormatResult};
+/// use
arrow_cast::pretty::{pretty_format_batches_with_options_and_formatters,
ArrayFormatterFactory};
+/// use arrow_schema::{ArrowError, Field};
+///
+/// /// A custom formatter factory that can create a formatter for the special
type `my_money`.
+/// ///
+/// /// This struct could have access to some kind of extension type registry
that can lookup the
+/// /// correct formatter for an extension type on-demand.
+/// struct MyFormatters {}
+///
+/// impl ArrayFormatterFactory for MyFormatters {
+/// fn create_display_index<'formatter>(
+/// &self,
+/// array: &'formatter dyn Array,
+/// options: &'formatter FormatOptions<'formatter>,
+/// field: Option<&'formatter Field>,
+/// ) -> Result<Option<ArrayFormatter<'formatter>>, ArrowError> {
+/// // check if this is the money type
+/// if field
+/// .map(|f| f.extension_type_name() == Some("my_money"))
+/// .unwrap_or(false)
+/// {
+/// // We assume that my_money always is an Int32.
+/// let array = array.as_primitive();
+/// let display_index = Box::new(MyMoneyFormatter { array, options
});
+/// return Ok(Some(ArrayFormatter::new(display_index,
options.safe)));
+/// }
+///
+/// Ok(None) // None indicates that the default formatter should be
used.
+/// }
+/// }
+///
+/// /// A formatter for the type `my_money` that wraps a specific array and
has access to the
Review Comment:
This is great -- thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
