adriangb commented on code in PR #11971:
URL: https://github.com/apache/arrow-rs/pull/11971#discussion_r4406621984
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -1404,11 +1471,12 @@ impl ArrowRowGroupWriterFactory {
file_writer: &SerializedFileWriter<W>,
arrow_schema: SchemaRef,
) -> Self {
- let schema = Arc::clone(file_writer.schema_descr_ptr());
+ let schema_plan =
+
ArrowWriteSchemaPlan::try_new(file_writer.schema_descr_ptr().as_ref(),
&arrow_schema)
+ .map(Arc::new);
Review Comment:
It seems like the ceremony of storing errors for later use is to avoid
changing the signature on this function (`ArrowRowGroupWriterFactory::new`,
which is public). But it feels to me that the right long-term shape is to add
`ArrowRowGroupWriterFactory::try_new(...) -> Result<...>`. I would suggest we
add that method and either:
(1) Remove `new()` as a hard breaking change (but an easy one to fix, I'd
guess 1 LOC in datafusion or any downstream). It should be fine to ship in next
major release.
(2) Keep `new()` and have it use this machinery internally, but deprecate it
and suggest using `try_new` instead. We could at least remove this complexity
in the future.
If we go with (2) we may have some bugs to fix. My agent claims that
`ParquetError::ArrowError` would end up in `General`. I did not verify this.
--
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]
