You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/02/11 18:31:14 UTC
[GitHub] [arrow-rs] sum12 opened a new pull request #1301: Json formatted datetime parsing
sum12 opened a new pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301
# 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.
-->
# Rationale for this change
<!---
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
The PR reuses the csv parser implementation for date64 (which implements date64 parsing using a format string) for json reader. However the json reader derives its values from the fields metadata. This is a different approach as compared to the csv parser and is more flexible as it gives the ability to have different format string for each field. With the need to have additional attr added to decoder/reader.
<!---
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
There is no change in the existing parsing flow, however Fields' metadata's key (`format_string`) is being hijacked for internal purposes. Not sure where to document this.
<!---
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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
[GitHub] [arrow-rs] sum12 commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r807998393
##########
File path: arrow/src/util/reader_parser.rs
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::*;
+
+/// Specialized parsing implementations
+/// used by csv and json reader
+pub trait Parser: ArrowPrimitiveType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ string.parse::<Self::Native>().ok()
+ }
+
+ fn parse_formatted(string: &str, _format: &str) -> Option<Self::Native> {
+ Self::parse(string)
+ }
+}
+
+/// Number of days between 0001-01-01 and 1970-01-01
+const EPOCH_DAYS_FROM_CE: i32 = 719_163;
+
+impl Parser for Date32Type {
+ fn parse(string: &str) -> Option<i32> {
+ use chrono::Datelike;
+
+ match Self::DATA_TYPE {
+ DataType::Date32 => {
+ let date = string.parse::<chrono::NaiveDate>().ok()?;
+ Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)
+ }
+ _ => None,
+ }
+ }
+
+ fn parse_formatted(string: &str, format: &str) -> Option<i32> {
Review comment:
ah, i think there is a mis-understanding here.
The method is **not** printing the date but parsing the string using the given format. I dont think https://docs.rs/chrono/latest/chrono/naive/struct.NaiveDate.html#method.format can be used here.
--
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
[GitHub] [arrow-rs] sum12 commented on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1054017817
Bump :-)
--
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
[GitHub] [arrow-rs] alamb commented on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1054319351
Sorry @sum12 -- I have been away for this last week. I'll put this PR on my queue to review more carefully shortly
--
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
[GitHub] [arrow-rs] sum12 commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r805389418
##########
File path: arrow/src/json/reader.rs
##########
@@ -942,6 +943,32 @@ impl Decoder {
))
}
+ #[allow(clippy::unnecessary_wraps)]
+ fn build_primitive_array_using_format_string<T>(
+ &self,
+ rows: &[Value],
+ col_name: &str,
+ format_string: &str,
+ ) -> Result<ArrayRef>
+ where
+ T: ArrowNumericType + Parser,
+ T::Native: num::NumCast,
+ {
+ Ok(Arc::new(
+ rows.iter()
+ .map(|row| {
+ row.get(&col_name).and_then(|value| {
+ if value.is_string() {
Review comment:
Thanks ! I ended up using `and_then` to avoid calling `flatten`. it definitely looks cleaner
--
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
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1036987553
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1301](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d47602) into [master](https://codecov.io/gh/apache/arrow-rs/commit/8f7c56e35f24580993b362e19ea7138c14824c0b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8f7c56e) will **increase** coverage by `0.03%`.
> The diff coverage is `70.21%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1301 +/- ##
==========================================
+ Coverage 83.01% 83.05% +0.03%
==========================================
Files 180 181 +1
Lines 52731 52873 +142
==========================================
+ Hits 43775 43912 +137
- Misses 8956 8961 +5
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/csv/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2Nzdi9yZWFkZXIucnM=) | `89.42% <ø> (+1.30%)` | :arrow_up: |
| [arrow/src/util/reader\_parser.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3V0aWwvcmVhZGVyX3BhcnNlci5ycw==) | `57.77% <57.77%> (ø)` | |
| [arrow/src/json/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2pzb24vcmVhZGVyLnJz) | `83.46% <81.63%> (+0.07%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.40% <0.00%> (-0.40%)` | :arrow_down: |
| [parquet/src/arrow/converter.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvY29udmVydGVyLnJz) | `63.96% <0.00%> (-0.39%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `65.98% <0.00%> (-0.23%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `84.51% <0.00%> (-0.13%)` | :arrow_down: |
| [arrow/src/array/array\_union.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3VuaW9uLnJz) | `90.71% <0.00%> (-0.05%)` | :arrow_down: |
| [parquet/src/arrow/arrow\_writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyb3dfd3JpdGVyLnJz) | `97.56% <0.00%> (-0.02%)` | :arrow_down: |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `86.73% <0.00%> (-0.01%)` | :arrow_down: |
| ... and [5 more](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8f7c56e...2d47602](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [arrow-rs] sum12 commented on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1067680375
Updated the PR with `format_strings` map parameter
--
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
[GitHub] [arrow-rs] alamb commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r805337330
##########
File path: arrow/src/json/reader.rs
##########
@@ -1875,6 +1926,15 @@ mod tests {
.unwrap();
assert!(2.0 - bb.value(0) < f32::EPSILON);
assert!(-3.5 - bb.value(1) < f32::EPSILON);
+ let ee = batch
+ .column(e.0)
+ .as_any()
+ .downcast_ref::<Date32Array>()
+ .unwrap();
+ dbg!(ee);
+ assert_eq!(1, ee.value(0));
+ assert_eq!(-1, ee.value(1));
+ assert!(!ee.is_valid(2));
Review comment:
Can you also check the value of index 2
If possible, it would also be helpful to have the tests in terms of the dates (aka compare to `1970-1-2` rather than a value 1 somehow)
--
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
[GitHub] [arrow-rs] codecov-commenter edited a comment on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1036987553
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1301](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d47602) into [master](https://codecov.io/gh/apache/arrow-rs/commit/8f7c56e35f24580993b362e19ea7138c14824c0b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8f7c56e) will **increase** coverage by `0.03%`.
> The diff coverage is `70.21%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1301 +/- ##
==========================================
+ Coverage 83.01% 83.05% +0.03%
==========================================
Files 180 181 +1
Lines 52731 52873 +142
==========================================
+ Hits 43775 43912 +137
- Misses 8956 8961 +5
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/csv/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2Nzdi9yZWFkZXIucnM=) | `89.42% <ø> (+1.30%)` | :arrow_up: |
| [arrow/src/util/reader\_parser.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3V0aWwvcmVhZGVyX3BhcnNlci5ycw==) | `57.77% <57.77%> (ø)` | |
| [arrow/src/json/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2pzb24vcmVhZGVyLnJz) | `83.46% <81.63%> (+0.07%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.40% <0.00%> (-0.40%)` | :arrow_down: |
| [parquet/src/arrow/converter.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvY29udmVydGVyLnJz) | `63.96% <0.00%> (-0.39%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `65.98% <0.00%> (-0.23%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `84.51% <0.00%> (-0.13%)` | :arrow_down: |
| [arrow/src/array/array\_union.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3VuaW9uLnJz) | `90.71% <0.00%> (-0.05%)` | :arrow_down: |
| [parquet/src/arrow/arrow\_writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyb3dfd3JpdGVyLnJz) | `97.56% <0.00%> (-0.02%)` | :arrow_down: |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `86.73% <0.00%> (-0.01%)` | :arrow_down: |
| ... and [5 more](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8f7c56e...2d47602](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [arrow-rs] sum12 commented on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1057151478
Awesome, thanks for the idea of extending `json::Reader::new()`. That way metadata remains free of pollution. I wonder if there there a better way to handle breaking API changes
--
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
[GitHub] [arrow-rs] codecov-commenter commented on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1036987553
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1301](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e16f427) into [master](https://codecov.io/gh/apache/arrow-rs/commit/8f7c56e35f24580993b362e19ea7138c14824c0b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8f7c56e) will **decrease** coverage by `0.00%`.
> The diff coverage is `67.44%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1301 +/- ##
==========================================
- Coverage 83.01% 83.01% -0.01%
==========================================
Files 180 181 +1
Lines 52731 52771 +40
==========================================
+ Hits 43775 43806 +31
- Misses 8956 8965 +9
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/csv/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2Nzdi9yZWFkZXIucnM=) | `89.42% <ø> (+1.30%)` | :arrow_up: |
| [arrow/src/util/reader\_parser.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3V0aWwvcmVhZGVyX3BhcnNlci5ycw==) | `57.77% <57.77%> (ø)` | |
| [arrow/src/json/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2pzb24vcmVhZGVyLnJz) | `83.37% <78.04%> (-0.03%)` | :arrow_down: |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `53.79% <0.00%> (-0.31%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `84.51% <0.00%> (-0.13%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `66.21% <0.00%> (ø)` | |
| [arrow/src/array/array.rs](https://codecov.io/gh/apache/arrow-rs/pull/1301/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5LnJz) | `85.67% <0.00%> (+0.26%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [8f7c56e...e16f427](https://codecov.io/gh/apache/arrow-rs/pull/1301?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [arrow-rs] alamb commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r816288190
##########
File path: arrow/src/csv/reader.rs
##########
@@ -711,68 +701,6 @@ impl Parser for Int16Type {}
impl Parser for Int8Type {}
Review comment:
is there a reason to leave these `Parser` impls in `reader.rs` rather than moving them to `reader_parser.rs` as well?
##########
File path: arrow/src/json/reader.rs
##########
@@ -1123,6 +1148,10 @@ impl Decoder {
.iter()
.filter(|field| projection.is_empty() || projection.contains(field.name()))
.map(|field| {
+ let format_string = match field.metadata() {
+ Some(meta) => meta.get("format_string"),
+ _ => None,
+ };
Review comment:
I don't think it matters, but I think you can rewrite this a bit more succinctly like this (untested):
```suggestion
let format_string = field.metadata()
.map(|meta| meta.get("format_string"));
```
##########
File path: arrow/src/json/reader.rs
##########
@@ -942,6 +943,30 @@ impl Decoder {
))
}
+ #[allow(clippy::unnecessary_wraps)]
+ fn build_primitive_array_using_format_string<T>(
+ &self,
+ rows: &[Value],
+ col_name: &str,
+ format_string: &str,
+ ) -> Result<ArrayRef>
+ where
+ T: ArrowNumericType + Parser,
+ T::Native: num::NumCast,
Review comment:
Do we need this trait bound? It may be left over from `build_primitive_array` 🤔
It seemed to compile just fine like this for me:
```suggestion
T: Parser,
```
##########
File path: arrow/src/json/reader.rs
##########
@@ -942,6 +943,30 @@ impl Decoder {
))
}
+ #[allow(clippy::unnecessary_wraps)]
Review comment:
I know this is just copy/pasted from `build_primitive_array`, but I think we could follow this clippy lint rather than ignore it (perhaps as a follow on PR)
##########
File path: arrow/src/json/reader.rs
##########
@@ -1123,6 +1148,10 @@ impl Decoder {
.iter()
.filter(|field| projection.is_empty() || projection.contains(field.name()))
.map(|field| {
+ let format_string = match field.metadata() {
+ Some(meta) => meta.get("format_string"),
Review comment:
I recommend making `"format_string"` a symbolic constant.
##########
File path: arrow/src/util/reader_parser.rs
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::*;
+
+/// Specialized parsing implementations
+/// used by csv and json reader
+pub trait Parser: ArrowPrimitiveType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ string.parse::<Self::Native>().ok()
+ }
+
+ fn parse_formatted(string: &str, _format: &str) -> Option<Self::Native> {
+ Self::parse(string)
+ }
+}
+
+/// Number of days between 0001-01-01 and 1970-01-01
+const EPOCH_DAYS_FROM_CE: i32 = 719_163;
+
+impl Parser for Date32Type {
+ fn parse(string: &str) -> Option<i32> {
+ use chrono::Datelike;
+
+ match Self::DATA_TYPE {
+ DataType::Date32 => {
+ let date = string.parse::<chrono::NaiveDate>().ok()?;
+ Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)
+ }
+ _ => None,
+ }
+ }
+
+ fn parse_formatted(string: &str, format: &str) -> Option<i32> {
+ use chrono::Datelike;
+ match Self::DATA_TYPE {
+ DataType::Date32 => {
+ let date = chrono::NaiveDate::parse_from_str(string, format).ok()?;
+ Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)
+ }
+ _ => None,
+ }
+ }
+}
+
+impl Parser for Date64Type {
+ fn parse(string: &str) -> Option<i64> {
+ match Self::DATA_TYPE {
+ DataType::Date64 => {
+ let date_time = string.parse::<chrono::NaiveDateTime>().ok()?;
+ Self::Native::from_i64(date_time.timestamp_millis())
+ }
+ _ => None,
+ }
+ }
+
+ fn parse_formatted(string: &str, format: &str) -> Option<i64> {
+ match Self::DATA_TYPE {
+ DataType::Date64 => {
+ use chrono::format::Fixed;
+ use chrono::format::StrftimeItems;
+ let fmt = StrftimeItems::new(format);
+ let has_zone = fmt.into_iter().any(|item| match item {
Review comment:
I realize this PR just moves this code around, but I wonder if we could reuse `string_to_timestamp_nanos` as a follow on PR
https://sourcegraph.com/github.com/apache/arrow-rs/-/blob/arrow/src/compute/kernels/cast_utils.rs?L69&subtree=true
##########
File path: arrow/src/json/reader.rs
##########
@@ -1875,6 +1926,15 @@ mod tests {
.unwrap();
assert!(2.0 - bb.value(0) < f32::EPSILON);
assert!(-3.5 - bb.value(1) < f32::EPSILON);
+ let ee = batch
+ .column(e.0)
+ .as_any()
+ .downcast_ref::<Date32Array>()
+ .unwrap();
+ dbg!(ee);
+ assert_eq!(1, ee.value(0));
+ assert_eq!(-1, ee.value(1));
+ assert!(!ee.is_valid(2));
Review comment:
The test looks good to me now
##########
File path: arrow/src/util/reader_parser.rs
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::*;
+
+/// Specialized parsing implementations
+/// used by csv and json reader
+pub trait Parser: ArrowPrimitiveType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ string.parse::<Self::Native>().ok()
+ }
+
+ fn parse_formatted(string: &str, _format: &str) -> Option<Self::Native> {
+ Self::parse(string)
+ }
+}
+
+/// Number of days between 0001-01-01 and 1970-01-01
+const EPOCH_DAYS_FROM_CE: i32 = 719_163;
+
+impl Parser for Date32Type {
+ fn parse(string: &str) -> Option<i32> {
+ use chrono::Datelike;
+
+ match Self::DATA_TYPE {
+ DataType::Date32 => {
+ let date = string.parse::<chrono::NaiveDate>().ok()?;
+ Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)
+ }
+ _ => None,
+ }
+ }
+
+ fn parse_formatted(string: &str, format: &str) -> Option<i32> {
Review comment:
Perhaps what @Dandandan was alluding to was to try and avoid the cost of parsing the format string for each element (e.g. try and reuse `StrftimeItems` somehow rather than recreating them all the time in https://docs.rs/chrono/latest/src/chrono/datetime.rs.html#386-390)
However, it is probably worth nothing that master already does the "parse the format string each time" so it is probably ok to leave that behavior in this PR (we could file a follow on issue for someone to look at if they are interested)
##########
File path: arrow/src/util/reader_parser.rs
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::*;
+
+/// Specialized parsing implementations
+/// used by csv and json reader
+pub trait Parser: ArrowPrimitiveType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ string.parse::<Self::Native>().ok()
+ }
+
+ fn parse_formatted(string: &str, _format: &str) -> Option<Self::Native> {
+ Self::parse(string)
+ }
+}
+
+/// Number of days between 0001-01-01 and 1970-01-01
+const EPOCH_DAYS_FROM_CE: i32 = 719_163;
+
+impl Parser for Date32Type {
+ fn parse(string: &str) -> Option<i32> {
+ use chrono::Datelike;
+
+ match Self::DATA_TYPE {
+ DataType::Date32 => {
+ let date = string.parse::<chrono::NaiveDate>().ok()?;
+ Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)
+ }
+ _ => None,
+ }
+ }
+
+ fn parse_formatted(string: &str, format: &str) -> Option<i32> {
+ use chrono::Datelike;
+ match Self::DATA_TYPE {
+ DataType::Date32 => {
+ let date = chrono::NaiveDate::parse_from_str(string, format).ok()?;
+ Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)
+ }
+ _ => None,
+ }
+ }
+}
+
+impl Parser for Date64Type {
+ fn parse(string: &str) -> Option<i64> {
+ match Self::DATA_TYPE {
+ DataType::Date64 => {
+ let date_time = string.parse::<chrono::NaiveDateTime>().ok()?;
+ Self::Native::from_i64(date_time.timestamp_millis())
+ }
+ _ => None,
+ }
+ }
+
+ fn parse_formatted(string: &str, format: &str) -> Option<i64> {
+ match Self::DATA_TYPE {
Review comment:
the match is strange here -- won't Self::DATA_TYPE always be `DataType::Date64`?
--
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
[GitHub] [arrow-rs] Dandandan commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r805416216
##########
File path: arrow/src/util/reader_parser.rs
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::*;
+
+/// Specialized parsing implementations
+/// used by csv and json reader
+pub trait Parser: ArrowPrimitiveType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ string.parse::<Self::Native>().ok()
+ }
+
+ fn parse_formatted(string: &str, _format: &str) -> Option<Self::Native> {
+ Self::parse(string)
+ }
+}
+
+/// Number of days between 0001-01-01 and 1970-01-01
+const EPOCH_DAYS_FROM_CE: i32 = 719_163;
+
+impl Parser for Date32Type {
+ fn parse(string: &str) -> Option<i32> {
+ use chrono::Datelike;
+
+ match Self::DATA_TYPE {
+ DataType::Date32 => {
+ let date = string.parse::<chrono::NaiveDate>().ok()?;
+ Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)
+ }
+ _ => None,
+ }
+ }
+
+ fn parse_formatted(string: &str, format: &str) -> Option<i32> {
Review comment:
For performance, it probably makes sense to reuse the formatter between values, by e.g. using the format function: https://docs.rs/chrono/latest/chrono/naive/struct.NaiveDate.html#method.format
--
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
[GitHub] [arrow-rs] sum12 commented on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1057224800
Moved the code shuffling part to a different PR. Hope it helps
--
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
[GitHub] [arrow-rs] sum12 commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r805389418
##########
File path: arrow/src/json/reader.rs
##########
@@ -942,6 +943,32 @@ impl Decoder {
))
}
+ #[allow(clippy::unnecessary_wraps)]
+ fn build_primitive_array_using_format_string<T>(
+ &self,
+ rows: &[Value],
+ col_name: &str,
+ format_string: &str,
+ ) -> Result<ArrayRef>
+ where
+ T: ArrowNumericType + Parser,
+ T::Native: num::NumCast,
+ {
+ Ok(Arc::new(
+ rows.iter()
+ .map(|row| {
+ row.get(&col_name).and_then(|value| {
+ if value.is_string() {
Review comment:
Thanks ! I ended up using `and_then` to avoid calling `flatten`. it definitely looks cleaner
##########
File path: arrow/src/json/reader.rs
##########
@@ -1875,6 +1926,15 @@ mod tests {
.unwrap();
assert!(2.0 - bb.value(0) < f32::EPSILON);
assert!(-3.5 - bb.value(1) < f32::EPSILON);
+ let ee = batch
+ .column(e.0)
+ .as_any()
+ .downcast_ref::<Date32Array>()
+ .unwrap();
+ dbg!(ee);
+ assert_eq!(1, ee.value(0));
+ assert_eq!(-1, ee.value(1));
+ assert!(!ee.is_valid(2));
Review comment:
Sure, good point. Ended up moving to a different test altogether.
Not sure how to "check" a null value (index 2 is null as it does not fit the format), or did you mean index 1 ?
--
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
[GitHub] [arrow-rs] Dandandan commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r805303279
##########
File path: arrow/src/json/reader.rs
##########
@@ -942,6 +943,32 @@ impl Decoder {
))
}
+ #[allow(clippy::unnecessary_wraps)]
+ fn build_primitive_array_using_format_string<T>(
+ &self,
+ rows: &[Value],
+ col_name: &str,
+ format_string: &str,
+ ) -> Result<ArrayRef>
+ where
+ T: ArrowNumericType + Parser,
+ T::Native: num::NumCast,
+ {
+ Ok(Arc::new(
+ rows.iter()
+ .map(|row| {
+ row.get(&col_name).and_then(|value| {
+ if value.is_string() {
Review comment:
Probably could be simplified a bit by using `value.as_str()` directly and pattern match or use `map` on the resulting value`
##########
File path: arrow/src/util/reader_parser.rs
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::*;
+
+/// Specialized parsing implementations
+/// used by csv and json reader
+pub trait Parser: ArrowPrimitiveType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ string.parse::<Self::Native>().ok()
+ }
+
+ fn parse_formatted(string: &str, _format: &str) -> Option<Self::Native> {
+ Self::parse(string)
+ }
+}
+
+/// Number of days between 0001-01-01 and 1970-01-01
+const EPOCH_DAYS_FROM_CE: i32 = 719_163;
+
+impl Parser for Date32Type {
+ fn parse(string: &str) -> Option<i32> {
+ use chrono::Datelike;
+
+ match Self::DATA_TYPE {
+ DataType::Date32 => {
+ let date = string.parse::<chrono::NaiveDate>().ok()?;
+ Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)
+ }
+ _ => None,
+ }
+ }
+
+ fn parse_formatted(string: &str, format: &str) -> Option<i32> {
Review comment:
For performance, it probably makes sense to reuse the formatter between values, by e.g. using the format function: https://docs.rs/chrono/latest/chrono/naive/struct.NaiveDate.html#method.format
--
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
[GitHub] [arrow-rs] alamb commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r805337330
##########
File path: arrow/src/json/reader.rs
##########
@@ -1875,6 +1926,15 @@ mod tests {
.unwrap();
assert!(2.0 - bb.value(0) < f32::EPSILON);
assert!(-3.5 - bb.value(1) < f32::EPSILON);
+ let ee = batch
+ .column(e.0)
+ .as_any()
+ .downcast_ref::<Date32Array>()
+ .unwrap();
+ dbg!(ee);
+ assert_eq!(1, ee.value(0));
+ assert_eq!(-1, ee.value(1));
+ assert!(!ee.is_valid(2));
Review comment:
Can you also check the value of index 2
If possible, it would also be helpful to have the tests in terms of the dates (aka compare to `1970-1-2` rather than a value 1 somehow)
--
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
[GitHub] [arrow-rs] sum12 commented on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1069090957
@alamb, In hopes on making it ligher to review, created a new PR. It probably is cleaner than this 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
[GitHub] [arrow-rs] Dandandan commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r805303279
##########
File path: arrow/src/json/reader.rs
##########
@@ -942,6 +943,32 @@ impl Decoder {
))
}
+ #[allow(clippy::unnecessary_wraps)]
+ fn build_primitive_array_using_format_string<T>(
+ &self,
+ rows: &[Value],
+ col_name: &str,
+ format_string: &str,
+ ) -> Result<ArrayRef>
+ where
+ T: ArrowNumericType + Parser,
+ T::Native: num::NumCast,
+ {
+ Ok(Arc::new(
+ rows.iter()
+ .map(|row| {
+ row.get(&col_name).and_then(|value| {
+ if value.is_string() {
Review comment:
Probably could be simplified a bit by using `value.as_str()` directly and pattern match or use `map` on the resulting value`
--
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
[GitHub] [arrow-rs] sum12 commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r805661804
##########
File path: arrow/src/util/reader_parser.rs
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::*;
+
+/// Specialized parsing implementations
+/// used by csv and json reader
+pub trait Parser: ArrowPrimitiveType {
+ fn parse(string: &str) -> Option<Self::Native> {
+ string.parse::<Self::Native>().ok()
+ }
+
+ fn parse_formatted(string: &str, _format: &str) -> Option<Self::Native> {
+ Self::parse(string)
+ }
+}
+
+/// Number of days between 0001-01-01 and 1970-01-01
+const EPOCH_DAYS_FROM_CE: i32 = 719_163;
+
+impl Parser for Date32Type {
+ fn parse(string: &str) -> Option<i32> {
+ use chrono::Datelike;
+
+ match Self::DATA_TYPE {
+ DataType::Date32 => {
+ let date = string.parse::<chrono::NaiveDate>().ok()?;
+ Self::Native::from_i32(date.num_days_from_ce() - EPOCH_DAYS_FROM_CE)
+ }
+ _ => None,
+ }
+ }
+
+ fn parse_formatted(string: &str, format: &str) -> Option<i32> {
Review comment:
Storing intermediate results is definitely a good idea. However not sure how to do such things in rust. Each `DataType` would have a different type for intermediate result. But `parse_formatted` (or anything new) would expect a unified interface. All these types could be unified under a `enum`, is that correct ? Not sure if that is what should be done, also it would be a breaking API change ?
--
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
[GitHub] [arrow-rs] sum12 commented on a change in pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
sum12 commented on a change in pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#discussion_r805389665
##########
File path: arrow/src/json/reader.rs
##########
@@ -1875,6 +1926,15 @@ mod tests {
.unwrap();
assert!(2.0 - bb.value(0) < f32::EPSILON);
assert!(-3.5 - bb.value(1) < f32::EPSILON);
+ let ee = batch
+ .column(e.0)
+ .as_any()
+ .downcast_ref::<Date32Array>()
+ .unwrap();
+ dbg!(ee);
+ assert_eq!(1, ee.value(0));
+ assert_eq!(-1, ee.value(1));
+ assert!(!ee.is_valid(2));
Review comment:
Sure, good point. Ended up moving to a different test altogether.
Not sure how to "check" a null value (index 2 is null as it does not fit the format), or did you mean index 1 ?
--
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
[GitHub] [arrow-rs] alamb closed pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301
--
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
[GitHub] [arrow-rs] alamb commented on pull request #1301: Json formatted datetime parsing
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1301:
URL: https://github.com/apache/arrow-rs/pull/1301#issuecomment-1082238564
Superceded by https://github.com/apache/arrow-rs/pull/1451 so closing this 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