paleolimbot commented on code in PR #8528:
URL: https://github.com/apache/arrow-rs/pull/8528#discussion_r2395440011
##########
parquet/src/basic.rs:
##########
@@ -928,17 +930,31 @@ enum BoundaryOrder {
// ----------------------------------------------------------------------
// Mirrors thrift enum `EdgeInterpolationAlgorithm`
+// TODO(ets): we need to allow for unknown variants. Either hand code this
one, or add a new
+// macro that adds an _Unknown variant.
Review Comment:
The algorithm is taken into account by the writer when writing the
stats...statistics for Geography are in theory safe to use for pruning even if
the algorithm is unrecognized (although it's difficult to imagine a situation
where this would occur except a corrupted file). I put `UNKNOWN` in the other
PR because I couldn't make the `From<>` implementation infallible without it
but perhaps you don't have that constraint here.
##########
parquet/src/basic.rs:
##########
@@ -456,9 +457,10 @@ impl<'a, R: ThriftCompactInputProtocol<'a>> ReadThrift<'a,
R> for LogicalType {
}
18 => {
let val = GeographyType::read_thrift(&mut *prot)?;
+ let algorithm = val.algorithm.unwrap_or_default();
Review Comment:
I don't mind either way...the interpretation of "the default" is part of the
Parquet standard (if unset in Thrift, the interpretation is spherical), my
thought was that this would make it so that others don't have to read the spec
in order to do the right thing.
--
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]
