[GitHub] [arrow-rs] alamb commented on a diff in pull request #1451: Allow json reader/decoder to work with format_strings for each field and use Parser trait correspondingly

[GitBox <gi...@apache.org>]

alamb commented on code in PR #1451:
URL: https://github.com/apache/arrow-rs/pull/1451#discussion_r847739816


##########
arrow/src/util/reader_parser.rs:
##########
@@ -60,27 +60,39 @@ impl Parser for Int8Type {}
 
 impl Parser for TimestampNanosecondType {
     fn parse(string: &str) -> Option<i64> {
-        match Self::DATA_TYPE {
-            DataType::Timestamp(TimeUnit::Nanosecond, None) => {
-                string_to_timestamp_nanos(string).ok()
-            }
-            _ => None,
-        }
+        string_to_timestamp_nanos(string).ok()
     }
 }
 
 impl Parser for TimestampMicrosecondType {
     fn parse(string: &str) -> Option<i64> {
-        match Self::DATA_TYPE {
-            DataType::Timestamp(TimeUnit::Microsecond, None) => {
-                let nanos = string_to_timestamp_nanos(string).ok();
-                nanos.map(|x| x / 1000)
-            }
-            _ => None,
-        }
+        let nanos = string_to_timestamp_nanos(string).ok();
+        nanos.map(|x| x / 1000)
+    }
+}
+
+impl Parser for TimestampMillisecondType {
+    fn parse(string: &str) -> Option<i64> {
+        let nanos = string_to_timestamp_nanos(string).ok();
+        nanos.map(|x| x / 1_000_000)
+    }
+}
+
+impl Parser for TimestampSecondType {
+    fn parse(string: &str) -> Option<i64> {
+        let nanos = string_to_timestamp_nanos(string).ok();
+        nanos.map(|x| x / 1_000_000_000)
     }
 }
 
+impl Parser for Time64NanosecondType {}

Review Comment:
   If we do this, doesn't it mean that integers will be parsed as time64s? Is that what you want to allow? If so can you please explain / write a test that covers the behavior (so we don't accidentally break it in the future?)



##########
arrow/src/json/reader.rs:
##########
@@ -577,30 +579,34 @@ pub struct Decoder {
     /// Explicit schema for the JSON file
     schema: SchemaRef,
     /// Optional projection for which columns to load (case-sensitive names)

Review Comment:
   I think you may have deleted the wrong comment here  the `/// Batch size ..` comment should remain and the `/// Optional projection...` should be removed



##########
arrow/src/json/reader.rs:
##########
@@ -577,30 +579,34 @@ pub struct Decoder {
     /// Explicit schema for the JSON file
     schema: SchemaRef,
     /// Optional projection for which columns to load (case-sensitive names)

Review Comment:
   I fixed it in 8427ce6e52



-- 
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