Jefffrey commented on code in PR #10840:
URL: https://github.com/apache/arrow-rs/pull/10840#discussion_r3963482998
##########
arrow-schema/src/datatype_parse.rs:
##########
@@ -251,6 +251,19 @@ impl<'a> Parser<'a> {
}
}
+ /// Parses the next double-quoted string
+ fn parse_quoted_string(&mut self, context: &str) -> ArrowResult<String> {
Review Comment:
this exists as `parse_double_quoted_string()`
##########
arrow-schema/src/datatype_display.rs:
##########
@@ -173,11 +174,34 @@ impl Display for DataType {
Ok(())
}
Self::RunEndEncoded(run_ends_field, values_field) => {
+ let default_names = run_ends_field.name() ==
Field::REE_RUN_ENDS_FIELD_DEFAULT_NAME
+ && values_field.name() ==
Field::REE_VALUES_FIELD_DEFAULT_NAME;
+ let no_metadata =
+ run_ends_field.metadata().is_empty() &&
values_field.metadata().is_empty();
write!(f, "RunEndEncoded(")?;
- let run_ends_str = format_field(run_ends_field);
- let values_str = format_field(values_field);
-
- write!(f, "{run_ends_str}, {values_str})")?;
+ let re_null = format_nullability(run_ends_field);
+ let v_null = format_nullability(values_field);
+ let re_meta = format_metadata(run_ends_field.metadata());
+ let v_meta = format_metadata(values_field.metadata());
+ if default_names && no_metadata {
+ write!(
+ f,
+ "{re_null}{}, {v_null}{})",
+ run_ends_field.data_type(),
+ values_field.data_type(),
+ )?;
+ } else {
+ write!(
Review Comment:
we can simplify this by using the existing `format_field()` function, e.g.
```rust
let default_names = run_ends_field.name() ==
Field::REE_RUN_ENDS_FIELD_DEFAULT_NAME
&& values_field.name() == Field::REE_VALUES_FIELD_DEFAULT_NAME;
let no_metadata =
run_ends_field.metadata().is_empty() &&
values_field.metadata().is_empty();
write!(f, "RunEndEncoded(")?;
if default_names && no_metadata {
let re_null = format_nullability(run_ends_field);
let v_null = format_nullability(values_field);
write!(
f,
"{re_null}{}, {v_null}{})",
run_ends_field.data_type(),
values_field.data_type(),
)?;
} else {
let run_ends_str = format_field(run_ends_field);
let values_str = format_field(values_field);
write!(f, "{run_ends_str}, {values_str})")?;
}
```
##########
arrow-schema/src/field.rs:
##########
@@ -162,6 +162,14 @@ impl Field {
///
/// See [Arrow
Spec](https://github.com/apache/arrow/blob/b19c4761b558ade94ae05743062d92aacedef10e/format/Schema.fbs#L127-L138))
pub const MAP_VALUE_FIELD_DEFAULT_NAME: &'static str = "value";
+ /// Default field name for the run-ends field for RunEndEncoded
+ ///
+ /// See [Arrow
Spec](https://arrow.apache.org/docs/format/Columnar.html#run-end-encoded-layout)
+ pub const REE_RUN_ENDS_FIELD_DEFAULT_NAME: &'static str = "run_ends";
+ /// Default field name for the values field for RunEndEncoded
+ ///
+ /// See [Arrow
Spec](https://arrow.apache.org/docs/format/Columnar.html#run-end-encoded-layout)
+ pub const REE_VALUES_FIELD_DEFAULT_NAME: &'static str = "values";
Review Comment:
in a followup PR we can wire this in to more places where its used, similar
to the map consts above
##########
arrow-schema/src/datatype_parse.rs:
##########
@@ -597,20 +610,52 @@ impl<'a> Parser<'a> {
}
}
- /// Parses the next RunEndEncoded (called after `RunEndEncoded` has been
consumed)
- /// E.g: RunEndEncoded("run_ends": UInt32, "values": nonnull Int32)
+ /// Parses the next RunEndEncoded (called after `RunEndEncoded` has been
consumed).
+ ///
+ /// Compact form (default field names): `RunEndEncoded(non-null Int32,
non-null Utf8)`
+ /// Verbose form (custom field names): `RunEndEncoded("re": Int32, "v":
non-null Utf8)`
fn parse_run_end_encoded(&mut self) -> ArrowResult<DataType> {
self.expect_token(Token::LParen)?;
- let run_ends = self.parse_field()?;
- self.expect_token(Token::Comma)?;
- let values = self.parse_field()?;
+
+ // Distinguish compact from verbose by peeking: verbose starts with a
double-quoted name.
+ let verbose = matches!(
+ self.tokenizer.peek(),
+ Some(Ok(Token::DoubleQuotedString(_)))
+ );
+
+ let (run_ends, values) = if verbose {
+ let run_ends = self.parse_ree_verbose_field()?;
+ self.expect_token(Token::Comma)?;
+ let values = self.parse_ree_verbose_field()?;
+ (run_ends.with_nullable(false), values)
Review Comment:
i do wonder with our approach here; should it be permissive and override to
the correct nullability, or more strict and explicitly fail
i looked at map parsing but it seems its too permissive and doesnt actually
check its of the right form (struct with non null keys)
so perhaps we can explore in a followup
--
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]
