You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/05/07 18:07:39 UTC
[PR] Improve error messages if schema hint mismatches with parquet scheam [arrow-rs]
alamb opened a new pull request, #7481:
URL: https://github.com/apache/arrow-rs/pull/7481
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Part of https://github.com/apache/arrow-rs/issues/7460
# Rationale for this change
Per https://github.com/apache/arrow-rs/pull/7479#issuecomment-2859515712 the error messages are pretty bad
# What changes are included in this PR?
1. Improve error messages
2. Add some `Debug` impls for the reader builders so I could use `unwrap_err`
# Are there any user-facing changes?
better errors, new Debug impls
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet schema [arrow-rs]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481#discussion_r2078283292
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -506,37 +525,47 @@ impl ArrowReaderMetadata {
// parquet_to_arrow_field_levels is expected to throw an error if the schemas have
// different lengths, but we check here to be safe.
if inferred_len != supplied_len {
- Err(arrow_err!(format!(
- "incompatible arrow schema, expected {} columns received {}",
+ return Err(arrow_err!(format!(
+ "Incompatible supplied Arrow schema: expected {} columns received {}",
inferred_len, supplied_len
- )))
- } else {
- let diff_fields: Vec<_> = supplied_schema
- .fields()
- .iter()
- .zip(fields.iter())
- .filter_map(|(field1, field2)| {
- if field1 != field2 {
- Some(field1.name().clone())
- } else {
- None
- }
- })
- .collect();
+ )));
+ }
- if !diff_fields.is_empty() {
- Err(ParquetError::ArrowError(format!(
- "incompatible arrow schema, the following fields could not be cast: [{}]",
- diff_fields.join(", ")
- )))
- } else {
- Ok(Self {
- metadata,
- schema: supplied_schema,
- fields: field_levels.levels.map(Arc::new),
- })
+ let mut errors = Vec::new();
Review Comment:
Hmm...this would mean that extension types can be cast implicitly to their storage (or perhaps the opposite, depending on which field metadata takes precedence). It is probably safer to fail, but not the end of the world because those errors will show up later (an error matching a signature if the extension metadata is dropped, or an error parsing bytes if unexpected content was given an extension type by accident). A true "user defined type" solution for DataFusion would be a place to handle this properly in some future (`field_common(field_a, field_b) -> Field`, `field_cast(array, array_field, common_field) -> ArrayRef`, or something).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet schema [arrow-rs]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481#discussion_r2078288130
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -506,37 +525,47 @@ impl ArrowReaderMetadata {
// parquet_to_arrow_field_levels is expected to throw an error if the schemas have
// different lengths, but we check here to be safe.
if inferred_len != supplied_len {
- Err(arrow_err!(format!(
- "incompatible arrow schema, expected {} columns received {}",
+ return Err(arrow_err!(format!(
+ "Incompatible supplied Arrow schema: expected {} columns received {}",
inferred_len, supplied_len
- )))
- } else {
- let diff_fields: Vec<_> = supplied_schema
- .fields()
- .iter()
- .zip(fields.iter())
- .filter_map(|(field1, field2)| {
- if field1 != field2 {
- Some(field1.name().clone())
- } else {
- None
- }
- })
- .collect();
+ )));
+ }
- if !diff_fields.is_empty() {
- Err(ParquetError::ArrowError(format!(
- "incompatible arrow schema, the following fields could not be cast: [{}]",
- diff_fields.join(", ")
- )))
- } else {
- Ok(Self {
- metadata,
- schema: supplied_schema,
- fields: field_levels.levels.map(Arc::new),
- })
+ let mut errors = Vec::new();
+
+ let field_iter = supplied_schema.fields().iter().zip(fields.iter());
+
+ for (field1, field2) in field_iter {
+ if field1.data_type() != field2.data_type() {
+ errors.push(format!(
+ "data type mismatch for field {}: requested {:?} but found {:?}",
+ field1.name(),
+ field1.data_type(),
+ field2.data_type()
+ ));
+ }
+ if field1.is_nullable() != field2.is_nullable() {
Review Comment:
Can a non-nullable field be cast to a nullable one?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet schema [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet scheam [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481#discussion_r2078209996
##########
parquet/src/arrow/arrow_reader/filter.rs:
##########
@@ -108,11 +109,18 @@ where
/// not contiguous.
///
/// [`RowSelection`]: crate::arrow::arrow_reader::RowSelection
+
pub struct RowFilter {
/// A list of [`ArrowPredicate`]
pub(crate) predicates: Vec<Box<dyn ArrowPredicate>>,
}
+impl Debug for RowFilter {
+ fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+ write!(f, "RowFilter {{ {} predicates: }}", self.predicates.len())
Review Comment:
ArrowPredicate doesn't implement `Debug`
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -506,37 +525,47 @@ impl ArrowReaderMetadata {
// parquet_to_arrow_field_levels is expected to throw an error if the schemas have
// different lengths, but we check here to be safe.
if inferred_len != supplied_len {
- Err(arrow_err!(format!(
- "incompatible arrow schema, expected {} columns received {}",
+ return Err(arrow_err!(format!(
+ "Incompatible supplied Arrow schema: expected {} columns received {}",
inferred_len, supplied_len
- )))
- } else {
- let diff_fields: Vec<_> = supplied_schema
- .fields()
- .iter()
- .zip(fields.iter())
- .filter_map(|(field1, field2)| {
- if field1 != field2 {
- Some(field1.name().clone())
- } else {
- None
- }
- })
- .collect();
+ )));
+ }
- if !diff_fields.is_empty() {
- Err(ParquetError::ArrowError(format!(
- "incompatible arrow schema, the following fields could not be cast: [{}]",
- diff_fields.join(", ")
- )))
- } else {
- Ok(Self {
- metadata,
- schema: supplied_schema,
- fields: field_levels.levels.map(Arc::new),
- })
+ let mut errors = Vec::new();
Review Comment:
Improving this message is the point of the PR
I also relaxed the check slightly so this will now allow the fields to differ in metadata where previously this would return an error. There is no test coverage for mismatched schemas
FYI @paleolimbot in case you have any wisdom to share here
##########
arrow-array/src/record_batch.rs:
##########
@@ -1073,8 +1073,8 @@ mod tests {
let a = Int64Array::from(vec![1, 2, 3, 4, 5]);
- let batch = RecordBatch::try_new(Arc::new(schema), vec![Arc::new(a)]);
- assert!(batch.is_err());
+ let err = RecordBatch::try_new(Arc::new(schema), vec![Arc::new(a)]).unwrap_err();
Review Comment:
drive by cleanup
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -3462,7 +3497,7 @@ mod tests {
Field::new("col2_valid", ArrowDataType::Int32, false),
Field::new("col3_invalid", ArrowDataType::Int32, false),
])),
- "Arrow: incompatible arrow schema, the following fields could not be cast: [col1_invalid, col3_invalid]",
+ "Arrow: Incompatible supplied Arrow schema: data type mismatch for field col1_invalid: requested Int32 but found Int64, data type mismatch for field col3_invalid: requested Int32 but found Int64",
Review Comment:
this is a pretty good example of the before / after error messages. I would feel much better trying to debug the new message than the old
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet schema [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481#issuecomment-2863690609
Thank you for your comments @paleolimbot
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet schema [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481#discussion_r2080096874
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -506,37 +525,47 @@ impl ArrowReaderMetadata {
// parquet_to_arrow_field_levels is expected to throw an error if the schemas have
// different lengths, but we check here to be safe.
if inferred_len != supplied_len {
- Err(arrow_err!(format!(
- "incompatible arrow schema, expected {} columns received {}",
+ return Err(arrow_err!(format!(
+ "Incompatible supplied Arrow schema: expected {} columns received {}",
inferred_len, supplied_len
- )))
- } else {
- let diff_fields: Vec<_> = supplied_schema
- .fields()
- .iter()
- .zip(fields.iter())
- .filter_map(|(field1, field2)| {
- if field1 != field2 {
- Some(field1.name().clone())
- } else {
- None
- }
- })
- .collect();
+ )));
+ }
- if !diff_fields.is_empty() {
- Err(ParquetError::ArrowError(format!(
- "incompatible arrow schema, the following fields could not be cast: [{}]",
- diff_fields.join(", ")
- )))
- } else {
- Ok(Self {
- metadata,
- schema: supplied_schema,
- fields: field_levels.levels.map(Arc::new),
- })
+ let mut errors = Vec::new();
Review Comment:
I think relaxing the check means that a user could supply the reader a schema that had metadata that was not present in the file and the reader will then read RecordBatches that have that metadata
I agree `field_cast` is the longer term right thing to do in DataFusion
In arrow-rs I think that field "casting" is happening during reading of parquet
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet schema [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481#discussion_r2080164055
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -506,37 +525,47 @@ impl ArrowReaderMetadata {
// parquet_to_arrow_field_levels is expected to throw an error if the schemas have
// different lengths, but we check here to be safe.
if inferred_len != supplied_len {
- Err(arrow_err!(format!(
- "incompatible arrow schema, expected {} columns received {}",
+ return Err(arrow_err!(format!(
+ "Incompatible supplied Arrow schema: expected {} columns received {}",
inferred_len, supplied_len
- )))
- } else {
- let diff_fields: Vec<_> = supplied_schema
- .fields()
- .iter()
- .zip(fields.iter())
- .filter_map(|(field1, field2)| {
- if field1 != field2 {
- Some(field1.name().clone())
- } else {
- None
- }
- })
- .collect();
+ )));
+ }
- if !diff_fields.is_empty() {
- Err(ParquetError::ArrowError(format!(
- "incompatible arrow schema, the following fields could not be cast: [{}]",
- diff_fields.join(", ")
- )))
- } else {
- Ok(Self {
- metadata,
- schema: supplied_schema,
- fields: field_levels.levels.map(Arc::new),
- })
+ let mut errors = Vec::new();
Review Comment:
I am put the metadata check back in perhaps and we can relax it when necessary.
I mostly was being lazy to avoid writing a test for it .
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -506,37 +525,47 @@ impl ArrowReaderMetadata {
// parquet_to_arrow_field_levels is expected to throw an error if the schemas have
// different lengths, but we check here to be safe.
if inferred_len != supplied_len {
- Err(arrow_err!(format!(
- "incompatible arrow schema, expected {} columns received {}",
+ return Err(arrow_err!(format!(
+ "Incompatible supplied Arrow schema: expected {} columns received {}",
inferred_len, supplied_len
- )))
- } else {
- let diff_fields: Vec<_> = supplied_schema
- .fields()
- .iter()
- .zip(fields.iter())
- .filter_map(|(field1, field2)| {
- if field1 != field2 {
- Some(field1.name().clone())
- } else {
- None
- }
- })
- .collect();
+ )));
+ }
- if !diff_fields.is_empty() {
- Err(ParquetError::ArrowError(format!(
- "incompatible arrow schema, the following fields could not be cast: [{}]",
- diff_fields.join(", ")
- )))
- } else {
- Ok(Self {
- metadata,
- schema: supplied_schema,
- fields: field_levels.levels.map(Arc::new),
- })
+ let mut errors = Vec::new();
Review Comment:
I can put the metadata check back in perhaps and we can relax it when necessary.
I mostly was being lazy to avoid writing a test for it .
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet schema [arrow-rs]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481#discussion_r2080152074
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -506,37 +525,47 @@ impl ArrowReaderMetadata {
// parquet_to_arrow_field_levels is expected to throw an error if the schemas have
// different lengths, but we check here to be safe.
if inferred_len != supplied_len {
- Err(arrow_err!(format!(
- "incompatible arrow schema, expected {} columns received {}",
+ return Err(arrow_err!(format!(
+ "Incompatible supplied Arrow schema: expected {} columns received {}",
inferred_len, supplied_len
- )))
- } else {
- let diff_fields: Vec<_> = supplied_schema
- .fields()
- .iter()
- .zip(fields.iter())
- .filter_map(|(field1, field2)| {
- if field1 != field2 {
- Some(field1.name().clone())
- } else {
- None
- }
- })
- .collect();
+ )));
+ }
- if !diff_fields.is_empty() {
- Err(ParquetError::ArrowError(format!(
- "incompatible arrow schema, the following fields could not be cast: [{}]",
- diff_fields.join(", ")
- )))
- } else {
- Ok(Self {
- metadata,
- schema: supplied_schema,
- fields: field_levels.levels.map(Arc::new),
- })
+ let mut errors = Vec::new();
Review Comment:
Probably the only correctness issue would be if the supplied schema had conflicting extension metadata (e.g., `unit: m` vs `unit: cm`). I am not sure that the current Parquet reader ever produces extension metadata (does it read the `ARROW:i_forget_the_exact_name key` and deserialize the schema?), so perhaps not an issue as long as somebody remembers this when it does.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet schema [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481#issuecomment-2897547295
Thank you for the review @etseidl
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Improve error messages if schema hint mismatches with parquet schema [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7481:
URL: https://github.com/apache/arrow-rs/pull/7481#issuecomment-2909204068
Thank you for the comments and reviews @paleolimbot and @etseidl
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org