AlyAbdelmoneim commented on code in PR #20253:
URL: https://github.com/apache/datafusion/pull/20253#discussion_r2806582081
##########
datafusion/core/src/datasource/physical_plan/parquet.rs:
##########
@@ -1342,18 +1344,18 @@ mod tests {
let time_units_and_expected = vec![
(
- None, // Same as "ns" time_unit
+ None, // default: None = "ns"
Review Comment:
The reason I initially proceeded with removing `Option` was this part of the
docstring in `config.rs`:
/// (reading) If set, parquet reader will read columns of
/// physical type int96 as originating from a different resolution
/// than nanosecond.
I interpreted this as implying that the default behavior is effectively
nanoseconds anyway, so making the field non-optional with a default of
`DFTimeUnit::Nanosecond` seemed safe.
After reviewing the failing tests more closely, it’s clear that `None` does
not behave the same as explicitly setting `ns`, and that `None` really means
“don’t apply coercion at all”.
I’ll revert this change and keep `Option<DFTimeUnit>` to preserve the
existing semantics.
Does that sound like the right approach to 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
