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 2021/11/11 11:14:14 UTC
[GitHub] [arrow-rs] liukun4515 opened a new pull request #941: ARROW-RS-926: support read decimal data from csv reader if user provide the schema with decimal data type
liukun4515 opened a new pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941
# 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.
-->
Closes #926
# Rationale for this change
1. support read decimal data from csv reader if user provide the schema with decimal data type
2. add function to convert the string format decimal data type to i128 data type
3. add test for `parse decimal data type`
<!---
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?
<!---
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?
<!---
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] liukun4515 commented on a change in pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#discussion_r750319158
##########
File path: arrow/src/csv/reader.rs
##########
@@ -769,20 +773,93 @@ fn build_decimal_array(
Ok(Arc::new(decimal_builder.finish()))
}
-// parse the string format decimal value to i128 format.
-// like "125.12" to 12512_i128.
+// Parse the string format decimal value to i128 format and checking the precision and scale.
+// The result i128 value can't be out of bounds.
+fn parse_decimal_with_parameter(s: &str, precision: usize, scale: usize) -> Result<i128> {
+ if PARSE_DECIMAL_RE.is_match(s) {
+ let mut offset = s.len();
+ let len = s.len();
+ // each byte is digit、'-' or '.'
+ let mut base = 1;
+
+ // handle the value after the '.' and meet the scale
+ let delimiter_position = s.find('.');
+ match delimiter_position {
+ None => {
+ // there is no '.'
+ base = 10_i128.pow(scale as u32);
+ }
+ Some(mid) => {
+ // there is the '.'
+ if len - mid >= scale + 1 {
+ // If the string value is "123.12345" and the scale is 2, we should just remain '.12' and drop the '345' value.
+ offset -= len - mid - 1 - scale;
+ } else {
+ // If the string value is "123.12" and the scale is 4, we should append '00' to the tail.
+ base = 10_i128.pow((scale + 1 + mid - len) as u32);
+ }
+ }
+ };
+
+ let bytes = s.as_bytes();
+ let mut negative = false;
+ let mut result: i128 = 0;
+
+ while offset > 0 {
Review comment:
Great suggestion!!!
I'm a rust learner, some rust code style are relatively new to me.
I think we fix it in the next pull request.
--
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 #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#discussion_r750465996
##########
File path: arrow/src/csv/reader.rs
##########
@@ -769,20 +773,93 @@ fn build_decimal_array(
Ok(Arc::new(decimal_builder.finish()))
}
-// parse the string format decimal value to i128 format.
-// like "125.12" to 12512_i128.
+// Parse the string format decimal value to i128 format and checking the precision and scale.
+// The result i128 value can't be out of bounds.
+fn parse_decimal_with_parameter(s: &str, precision: usize, scale: usize) -> Result<i128> {
+ if PARSE_DECIMAL_RE.is_match(s) {
+ let mut offset = s.len();
+ let len = s.len();
+ // each byte is digit、'-' or '.'
+ let mut base = 1;
+
+ // handle the value after the '.' and meet the scale
+ let delimiter_position = s.find('.');
+ match delimiter_position {
+ None => {
+ // there is no '.'
+ base = 10_i128.pow(scale as u32);
+ }
+ Some(mid) => {
+ // there is the '.'
+ if len - mid >= scale + 1 {
+ // If the string value is "123.12345" and the scale is 2, we should just remain '.12' and drop the '345' value.
+ offset -= len - mid - 1 - scale;
+ } else {
+ // If the string value is "123.12" and the scale is 4, we should append '00' to the tail.
+ base = 10_i128.pow((scale + 1 + mid - len) as u32);
+ }
+ }
+ };
+
+ let bytes = s.as_bytes();
+ let mut negative = false;
+ let mut result: i128 = 0;
+
+ while offset > 0 {
Review comment:
👍 yeah it took me a while to get into the Rust / functional style
--
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] liukun4515 commented on a change in pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#discussion_r750316078
##########
File path: arrow/src/csv/reader.rs
##########
@@ -769,20 +773,93 @@ fn build_decimal_array(
Ok(Arc::new(decimal_builder.finish()))
}
-// parse the string format decimal value to i128 format.
-// like "125.12" to 12512_i128.
+// Parse the string format decimal value to i128 format and checking the precision and scale.
+// The result i128 value can't be out of bounds.
+fn parse_decimal_with_parameter(s: &str, precision: usize, scale: usize) -> Result<i128> {
+ if PARSE_DECIMAL_RE.is_match(s) {
+ let mut offset = s.len();
+ let len = s.len();
+ // each byte is digit、'-' or '.'
+ let mut base = 1;
+
+ // handle the value after the '.' and meet the scale
+ let delimiter_position = s.find('.');
Review comment:
I think this value `123.123.13` will get an error, because of the regex `static ref PARSE_DECIMAL_RE: Regex =
Regex::new(r"^-?(\d+\.?\d*|\d*\.?\d+)$").unwrap();`.
I will add more test case and change the code style in the next pull request.
--
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] liukun4515 commented on a change in pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#discussion_r749046645
##########
File path: arrow/src/csv/reader.rs
##########
@@ -728,6 +732,87 @@ fn parse_bool(string: &str) -> Option<bool> {
}
}
+// parse the column string to an Arrow Array
+fn build_decimal_array(
+ line_number: usize,
+ rows: &[StringRecord],
+ col_idx: usize,
+ precision: usize,
+ scale: usize,
+) -> Result<ArrayRef> {
+ let mut decimal_builder = DecimalBuilder::new(line_number, precision, scale);
+ for row in rows {
+ let col_s = row.get(col_idx);
+ match col_s {
+ None => {
+ // No data for this row
+ decimal_builder.append_null()?;
+ }
+ Some(s) => {
+ if s.is_empty() {
+ // append null
+ decimal_builder.append_null()?;
+ } else {
+ let decimal_value: Result<i128> = parse_decimal(s);
+ match decimal_value {
+ Ok(v) => {
+ decimal_builder.append_value(v)?;
+ }
+ Err(e) => {
+ return Err(e);
+ }
+ }
+ }
+ }
+ }
+ }
+ Ok(Arc::new(decimal_builder.finish()))
+}
+
+// parse the string format decimal value to i128 format.
+// like "125.12" to 12512_i128.
+fn parse_decimal(s: &str) -> Result<i128> {
+ if DECIMAL_RE.is_match(s) {
+ let mut offset = s.len();
+ // each byte is digit、'-' or '.'
+ let bytes = s.as_bytes();
+ let mut negitive = false;
+ let mut result: i128 = 0;
Review comment:
This is a simple way to convert a string to i128, but we may meet some corner case.
For example, we want to convert the value to decimal(10,2).
The string value (12.0000000000000000000000000000000000....000) (too may 'zero' after the decimal), if we remove the '.' and convert the value to i128, we may meet the overflow error.
--
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] liukun4515 commented on a change in pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#discussion_r750319158
##########
File path: arrow/src/csv/reader.rs
##########
@@ -769,20 +773,93 @@ fn build_decimal_array(
Ok(Arc::new(decimal_builder.finish()))
}
-// parse the string format decimal value to i128 format.
-// like "125.12" to 12512_i128.
+// Parse the string format decimal value to i128 format and checking the precision and scale.
+// The result i128 value can't be out of bounds.
+fn parse_decimal_with_parameter(s: &str, precision: usize, scale: usize) -> Result<i128> {
+ if PARSE_DECIMAL_RE.is_match(s) {
+ let mut offset = s.len();
+ let len = s.len();
+ // each byte is digit、'-' or '.'
+ let mut base = 1;
+
+ // handle the value after the '.' and meet the scale
+ let delimiter_position = s.find('.');
+ match delimiter_position {
+ None => {
+ // there is no '.'
+ base = 10_i128.pow(scale as u32);
+ }
+ Some(mid) => {
+ // there is the '.'
+ if len - mid >= scale + 1 {
+ // If the string value is "123.12345" and the scale is 2, we should just remain '.12' and drop the '345' value.
+ offset -= len - mid - 1 - scale;
+ } else {
+ // If the string value is "123.12" and the scale is 4, we should append '00' to the tail.
+ base = 10_i128.pow((scale + 1 + mid - len) as u32);
+ }
+ }
+ };
+
+ let bytes = s.as_bytes();
+ let mut negative = false;
+ let mut result: i128 = 0;
+
+ while offset > 0 {
Review comment:
Great suggestion!!!
I'm a rust learner, some rust code style are relatively new to me.
I think we can fix it in the next pull request.
--
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 #941: ARROW-RS-926: support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-966226692
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/941?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 [#941](https://codecov.io/gh/apache/arrow-rs/pull/941?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (544ffaf) into [master](https://codecov.io/gh/apache/arrow-rs/commit/1945f6f96551e501010a4578295456a1f9f335e9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1945f6f) will **increase** coverage by `0.01%`.
> The diff coverage is `88.40%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/941?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 #941 +/- ##
==========================================
+ Coverage 82.34% 82.36% +0.01%
==========================================
Files 168 168
Lines 48520 48610 +90
==========================================
+ Hits 39955 40038 +83
- Misses 8565 8572 +7
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/941?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/941/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.89% <88.40%> (-0.14%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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/compute/kernels/take.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy90YWtlLnJz) | `95.22% <0.00%> (+0.14%)` | :arrow_up: |
| [arrow/src/error.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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-YXJyb3cvc3JjL2Vycm9yLnJz) | `15.55% <0.00%> (+2.22%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/941?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/941?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 [1945f6f...544ffaf](https://codecov.io/gh/apache/arrow-rs/pull/941?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] liukun4515 commented on pull request #941: ARROW-RS-926: support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-966903809
@alamb PTAL
--
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 #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#discussion_r749359536
##########
File path: arrow/src/csv/reader.rs
##########
@@ -1503,6 +1586,54 @@ mod tests {
}
}
+ #[test]
+ fn test_parse_decimal_with_parameter() {
+ let tests = [
+ ("123.123", 123123i128),
+ ("123.1234", 123123i128),
+ ("123.1", 123100i128),
+ ("123", 123000i128),
+ ("-123.123", -123123i128),
+ ("-123.1234", -123123i128),
+ ("-123.1", -123100i128),
+ ("-123", -123000i128),
+ ("0.0000123", 0i128),
+ ("12.", 12000i128),
+ ("-12.", -12000i128),
+ ("00.1", 100i128),
+ ("-00.1", -100i128),
+ ("12345678912345678.1234", 12345678912345678123i128),
+ ("-12345678912345678.1234", -12345678912345678123i128),
+ ("99999999999999999.999", 99999999999999999999i128),
+ ("-99999999999999999.999", -99999999999999999999i128),
+ (".123", 123i128),
+ ("-.123", -123i128),
+ ("123.", 123000i128),
+ ("-123.", -123000i128),
+ ];
+ for (s, i) in tests {
+ let result = parse_decimal_with_parameter(s, 20, 3);
+ assert_eq!(i, result.unwrap())
+ }
+ let can_not_parse_tests = ["123,123", "."];
Review comment:
I recommend a test with two `.` in it as well, such as `123.123.123`
##########
File path: arrow/src/csv/reader.rs
##########
@@ -769,20 +773,93 @@ fn build_decimal_array(
Ok(Arc::new(decimal_builder.finish()))
}
-// parse the string format decimal value to i128 format.
-// like "125.12" to 12512_i128.
+// Parse the string format decimal value to i128 format and checking the precision and scale.
+// The result i128 value can't be out of bounds.
+fn parse_decimal_with_parameter(s: &str, precision: usize, scale: usize) -> Result<i128> {
+ if PARSE_DECIMAL_RE.is_match(s) {
+ let mut offset = s.len();
+ let len = s.len();
+ // each byte is digit、'-' or '.'
+ let mut base = 1;
+
+ // handle the value after the '.' and meet the scale
+ let delimiter_position = s.find('.');
Review comment:
I wonder what will happen if there are two `'.'` in the string - like `123.456.789` ( I would expect a parsing error in this case)
##########
File path: arrow/src/csv/reader.rs
##########
@@ -769,20 +773,93 @@ fn build_decimal_array(
Ok(Arc::new(decimal_builder.finish()))
}
-// parse the string format decimal value to i128 format.
-// like "125.12" to 12512_i128.
+// Parse the string format decimal value to i128 format and checking the precision and scale.
+// The result i128 value can't be out of bounds.
+fn parse_decimal_with_parameter(s: &str, precision: usize, scale: usize) -> Result<i128> {
+ if PARSE_DECIMAL_RE.is_match(s) {
+ let mut offset = s.len();
+ let len = s.len();
+ // each byte is digit、'-' or '.'
+ let mut base = 1;
+
+ // handle the value after the '.' and meet the scale
+ let delimiter_position = s.find('.');
+ match delimiter_position {
+ None => {
+ // there is no '.'
+ base = 10_i128.pow(scale as u32);
+ }
+ Some(mid) => {
+ // there is the '.'
+ if len - mid >= scale + 1 {
+ // If the string value is "123.12345" and the scale is 2, we should just remain '.12' and drop the '345' value.
+ offset -= len - mid - 1 - scale;
+ } else {
+ // If the string value is "123.12" and the scale is 4, we should append '00' to the tail.
+ base = 10_i128.pow((scale + 1 + mid - len) as u32);
+ }
+ }
+ };
+
+ let bytes = s.as_bytes();
+ let mut negative = false;
+ let mut result: i128 = 0;
+
+ while offset > 0 {
Review comment:
Something that might make this code faster (and more idomatic rust) would be to avoid the `bytes[offset - 1]` call (which does a bounds check).
It might look something like this (untested):
```rust
bytes[0..offset].iter().rev()
.try_for_each(|b| {
match b {
b'-' => negative = true,
b'.' => {},
...
}
})?;
##########
File path: arrow/src/csv/reader.rs
##########
@@ -1503,6 +1586,54 @@ mod tests {
}
}
+ #[test]
+ fn test_parse_decimal_with_parameter() {
Review comment:
👍
--
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] liukun4515 commented on a change in pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#discussion_r750319158
##########
File path: arrow/src/csv/reader.rs
##########
@@ -769,20 +773,93 @@ fn build_decimal_array(
Ok(Arc::new(decimal_builder.finish()))
}
-// parse the string format decimal value to i128 format.
-// like "125.12" to 12512_i128.
+// Parse the string format decimal value to i128 format and checking the precision and scale.
+// The result i128 value can't be out of bounds.
+fn parse_decimal_with_parameter(s: &str, precision: usize, scale: usize) -> Result<i128> {
+ if PARSE_DECIMAL_RE.is_match(s) {
+ let mut offset = s.len();
+ let len = s.len();
+ // each byte is digit、'-' or '.'
+ let mut base = 1;
+
+ // handle the value after the '.' and meet the scale
+ let delimiter_position = s.find('.');
+ match delimiter_position {
+ None => {
+ // there is no '.'
+ base = 10_i128.pow(scale as u32);
+ }
+ Some(mid) => {
+ // there is the '.'
+ if len - mid >= scale + 1 {
+ // If the string value is "123.12345" and the scale is 2, we should just remain '.12' and drop the '345' value.
+ offset -= len - mid - 1 - scale;
+ } else {
+ // If the string value is "123.12" and the scale is 4, we should append '00' to the tail.
+ base = 10_i128.pow((scale + 1 + mid - len) as u32);
+ }
+ }
+ };
+
+ let bytes = s.as_bytes();
+ let mut negative = false;
+ let mut result: i128 = 0;
+
+ while offset > 0 {
Review comment:
Great suggestion!!!
I'm a rust learner, some rust code style are relatively new to me.
--
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] liukun4515 commented on pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-968194554
> Thank you for the contribution @liukun4515 -- this is looking great.
>
> I had a question on the handling of decimals when the input doesn't have the same number of values as the declared scale
thanks for your comments, i also have the same question.
I will do some investigation about decimal in other DB system, and reply the question.
--
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 #941: ARROW-RS-926: support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-966226692
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/941?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 [#941](https://codecov.io/gh/apache/arrow-rs/pull/941?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cbac920) into [master](https://codecov.io/gh/apache/arrow-rs/commit/1945f6f96551e501010a4578295456a1f9f335e9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1945f6f) will **increase** coverage by `0.00%`.
> The diff coverage is `88.40%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/941?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 #941 +/- ##
=======================================
Coverage 82.34% 82.35%
=======================================
Files 168 168
Lines 48520 48589 +69
=======================================
+ Hits 39955 40015 +60
- Misses 8565 8574 +9
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/941?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/941/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.89% <88.40%> (-0.14%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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/941/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=) | `85.33% <0.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/941?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/941?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 [1945f6f...cbac920](https://codecov.io/gh/apache/arrow-rs/pull/941?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] liukun4515 commented on a change in pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#discussion_r749153410
##########
File path: arrow/src/csv/reader.rs
##########
@@ -1055,6 +1140,31 @@ mod tests {
assert_eq!(&metadata, batch.schema().metadata());
}
+ #[test]
+ fn test_csv_reader_with_decimal() {
+ let schema = Schema::new(vec![
+ Field::new("city", DataType::Utf8, false),
+ Field::new("lat", DataType::Decimal(26, 6), false),
+ Field::new("lng", DataType::Decimal(26, 6), false),
+ ]);
+
+ let file = File::open("test/data/uk_cities.csv").unwrap();
+
+ let mut csv = Reader::new(file, Arc::new(schema), false, None, 1024, None, None);
+ let batch = csv.next().unwrap().unwrap();
+ // access data from a primitive array
+ let lat = batch
+ .column(1)
+ .as_any()
+ .downcast_ref::<DecimalArray>()
+ .unwrap();
+
+ assert_eq!("57.653484", lat.value_as_string(0));
+ assert_eq!("53.002666", lat.value_as_string(1));
+ assert_eq!("52.412811", lat.value_as_string(2));
+ assert_eq!("51.481583", lat.value_as_string(3))
+ }
Review comment:
I have added other test cases to ensure right for above case.
--
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] liukun4515 commented on pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-970315466
@alamb maybe you merge this pr first.
I will add the test cases you mentioned in the next pr with the code style.
--
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] liukun4515 commented on pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-976095605
@alamb don't forget cherry pick this pull request with #952
--
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] liukun4515 commented on pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-968576937
Hi @alamb , I have checked the mysql and pg database.
We have two problems which should be discussed.
**The first is about scale, if the input string doesn't have the same value for the declared scale, how do we to handle this?**
In the PG:
```
postgres=# select cast('123' as decimal(10,2));
numeric
---------
123.00
(1 row)
postgres=# select cast('123.123213321' as decimal(10,2));
numeric
---------
123.12
(1 row)
```
In the MYSQL:
```
mysql> select cast('123' as decimal(10,2));
+------------------------------+
| cast('123' as decimal(10,2)) |
+------------------------------+
| 123.00 |
+------------------------------+
1 row in set (0.00 sec)
mysql> select cast('123.1' as decimal(10,2));
+--------------------------------+
| cast('123.1' as decimal(10,2)) |
+--------------------------------+
| 123.10 |
+--------------------------------+
1 row in set (0.00 sec)
mysql> select cast('123.12345' as decimal(10,2));
+------------------------------------+
| cast('123.12345' as decimal(10,2)) |
+------------------------------------+
| 123.12 |
+------------------------------------+
1 row in set (0.00 sec)
```
From the two database, we can know that the number of value after the decimal must match the scale parameter.
If the number of value is less than the scale, we should fill the `0` in the tail.
If the number of value is greater than the scale, we should truncate the tail to meet the scale.
**The second is about precision, if the input string value is out of bounds for the declared precision, how do we to handle this?**
This is the result for PG
```
postgres=# select cast('123' as decimal(2,2));
ERROR: numeric field overflow
DETAIL: A field with precision 2, scale 2 must round to an absolute value less than 1.
```
This is the result for mysql
```
mysql> select cast('123.12345' as decimal(2,2));
+-----------------------------------+
| cast('123.12345' as decimal(2,2)) |
+-----------------------------------+
| 0.99 |
+-----------------------------------+
1 row in set, 1 warning (0.00 sec)
mysql> show warnings;
+---------+------+----------------------------------------------------------------------------+
| Level | Code | Message |
+---------+------+----------------------------------------------------------------------------+
| Warning | 1264 | Out of range value for column 'cast('123.12345' as decimal(2,2))' at row 1 |
+---------+------+----------------------------------------------------------------------------+
1 row in set (0.00 sec)
```
In the PG and mysql, if the input string value is out of bounds for the precision, we will get some error.
--
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] liukun4515 edited a comment on pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
liukun4515 edited a comment on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-968576937
Hi @alamb , I have checked the mysql and pg database.
We have two problems which should be discussed.
**The first is about scale, if the input string doesn't have the same value for the declared scale, what should we do to handle this?**
In the PG:
```
postgres=# select cast('123' as decimal(10,2));
numeric
---------
123.00
(1 row)
postgres=# select cast('123.123213321' as decimal(10,2));
numeric
---------
123.12
(1 row)
```
In the MYSQL:
```
mysql> select cast('123' as decimal(10,2));
+------------------------------+
| cast('123' as decimal(10,2)) |
+------------------------------+
| 123.00 |
+------------------------------+
1 row in set (0.00 sec)
mysql> select cast('123.1' as decimal(10,2));
+--------------------------------+
| cast('123.1' as decimal(10,2)) |
+--------------------------------+
| 123.10 |
+--------------------------------+
1 row in set (0.00 sec)
mysql> select cast('123.12345' as decimal(10,2));
+------------------------------------+
| cast('123.12345' as decimal(10,2)) |
+------------------------------------+
| 123.12 |
+------------------------------------+
1 row in set (0.00 sec)
```
From the two database, we can know that the number of value after the decimal must match the scale parameter.
If the number of value is less than the scale, we should fill the `0` in the tail.
If the number of value is greater than the scale, we should truncate the tail to meet the scale.
**The second is about precision, if the input string value is out of bounds for the declared precision, what should we do to handle this?**
This is the result for PG
```
postgres=# select cast('123' as decimal(2,2));
ERROR: numeric field overflow
DETAIL: A field with precision 2, scale 2 must round to an absolute value less than 1.
```
This is the result for mysql
```
mysql> select cast('123.12345' as decimal(2,2));
+-----------------------------------+
| cast('123.12345' as decimal(2,2)) |
+-----------------------------------+
| 0.99 |
+-----------------------------------+
1 row in set, 1 warning (0.00 sec)
mysql> show warnings;
+---------+------+----------------------------------------------------------------------------+
| Level | Code | Message |
+---------+------+----------------------------------------------------------------------------+
| Warning | 1264 | Out of range value for column 'cast('123.12345' as decimal(2,2))' at row 1 |
+---------+------+----------------------------------------------------------------------------+
1 row in set (0.00 sec)
```
In the PG and mysql, if the input string value is out of bounds for the precision, we will get some error.
--
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 #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-966226692
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/941?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 [#941](https://codecov.io/gh/apache/arrow-rs/pull/941?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e93d85d) into [master](https://codecov.io/gh/apache/arrow-rs/commit/f5a4341c390c18f2e179ee73bc861d74ff7fc56a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f5a4341) will **increase** coverage by `0.03%`.
> The diff coverage is `92.19%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/941?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 #941 +/- ##
==========================================
+ Coverage 82.36% 82.40% +0.03%
==========================================
Files 168 168
Lines 48587 48728 +141
==========================================
+ Hits 40020 40153 +133
- Misses 8567 8575 +8
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/941?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/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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.64% <ø> (ø)` | |
| [arrow/src/csv/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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=) | `90.39% <92.19%> (+0.33%)` | :arrow_up: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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=) | `85.33% <0.00%> (ø)` | |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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%> (ø)` | |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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.68% <0.00%> (+0.30%)` | :arrow_up: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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.38% <0.00%> (+0.42%)` | :arrow_up: |
| [arrow/src/error.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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-YXJyb3cvc3JjL2Vycm9yLnJz) | `17.77% <0.00%> (+2.22%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/941?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/941?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 [f5a4341...e93d85d](https://codecov.io/gh/apache/arrow-rs/pull/941?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 #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#discussion_r748224224
##########
File path: arrow/src/csv/reader.rs
##########
@@ -513,6 +514,9 @@ fn parse(
let field = &fields[i];
match field.data_type() {
DataType::Boolean => build_boolean_array(line_number, rows, i),
+ DataType::Decimal(p, v) => {
Review comment:
I think using `DataType::Decimal(precision, scale)` rather than `DataType::Decimal(p, v)` would be more consistent with the rest of the codebase
##########
File path: arrow/src/csv/reader.rs
##########
@@ -728,6 +732,87 @@ fn parse_bool(string: &str) -> Option<bool> {
}
}
+// parse the column string to an Arrow Array
+fn build_decimal_array(
+ line_number: usize,
+ rows: &[StringRecord],
+ col_idx: usize,
+ precision: usize,
+ scale: usize,
+) -> Result<ArrayRef> {
+ let mut decimal_builder = DecimalBuilder::new(line_number, precision, scale);
+ for row in rows {
+ let col_s = row.get(col_idx);
+ match col_s {
+ None => {
+ // No data for this row
+ decimal_builder.append_null()?;
+ }
+ Some(s) => {
+ if s.is_empty() {
+ // append null
+ decimal_builder.append_null()?;
+ } else {
+ let decimal_value: Result<i128> = parse_decimal(s);
+ match decimal_value {
+ Ok(v) => {
+ decimal_builder.append_value(v)?;
+ }
+ Err(e) => {
+ return Err(e);
+ }
+ }
+ }
+ }
+ }
+ }
+ Ok(Arc::new(decimal_builder.finish()))
+}
+
+// parse the string format decimal value to i128 format.
Review comment:
I am surprised that we don't already have a `&str` --> i128 parser, but I also could not find one.
##########
File path: arrow/src/csv/reader.rs
##########
@@ -1055,6 +1140,31 @@ mod tests {
assert_eq!(&metadata, batch.schema().metadata());
}
+ #[test]
+ fn test_csv_reader_with_decimal() {
+ let schema = Schema::new(vec![
+ Field::new("city", DataType::Utf8, false),
+ Field::new("lat", DataType::Decimal(26, 6), false),
+ Field::new("lng", DataType::Decimal(26, 6), false),
+ ]);
+
+ let file = File::open("test/data/uk_cities.csv").unwrap();
+
+ let mut csv = Reader::new(file, Arc::new(schema), false, None, 1024, None, None);
+ let batch = csv.next().unwrap().unwrap();
+ // access data from a primitive array
+ let lat = batch
+ .column(1)
+ .as_any()
+ .downcast_ref::<DecimalArray>()
+ .unwrap();
+
+ assert_eq!("57.653484", lat.value_as_string(0));
+ assert_eq!("53.002666", lat.value_as_string(1));
+ assert_eq!("52.412811", lat.value_as_string(2));
+ assert_eq!("51.481583", lat.value_as_string(3))
+ }
Review comment:
Could you also add some tests where the full precision is not supplied? For example, try parsing
```
57.65
```
(only 2 places after the decimal)
I think this implementation may end up producing a decimal of `0.005765` rather than `57.65000`
Also, we should test
```
56.3838748284
```
(or something else that has more data after the decimal than the declared scale)
##########
File path: arrow/src/csv/reader.rs
##########
@@ -728,6 +732,87 @@ fn parse_bool(string: &str) -> Option<bool> {
}
}
+// parse the column string to an Arrow Array
+fn build_decimal_array(
+ line_number: usize,
+ rows: &[StringRecord],
+ col_idx: usize,
+ precision: usize,
+ scale: usize,
+) -> Result<ArrayRef> {
+ let mut decimal_builder = DecimalBuilder::new(line_number, precision, scale);
Review comment:
the first argument to `DecimalBuilder` is the capacity (aka the expected size of the array). So I think it would be more performant to use `rows.size()` rather than `line_number` here
https://docs.rs/arrow/6.1.0/arrow/array/struct.DecimalBuilder.html#method.new
##########
File path: arrow/src/csv/reader.rs
##########
@@ -728,6 +732,87 @@ fn parse_bool(string: &str) -> Option<bool> {
}
}
+// parse the column string to an Arrow Array
+fn build_decimal_array(
+ line_number: usize,
+ rows: &[StringRecord],
+ col_idx: usize,
+ precision: usize,
+ scale: usize,
+) -> Result<ArrayRef> {
+ let mut decimal_builder = DecimalBuilder::new(line_number, precision, scale);
+ for row in rows {
+ let col_s = row.get(col_idx);
+ match col_s {
+ None => {
+ // No data for this row
+ decimal_builder.append_null()?;
+ }
+ Some(s) => {
+ if s.is_empty() {
+ // append null
+ decimal_builder.append_null()?;
+ } else {
+ let decimal_value: Result<i128> = parse_decimal(s);
+ match decimal_value {
+ Ok(v) => {
+ decimal_builder.append_value(v)?;
+ }
+ Err(e) => {
+ return Err(e);
+ }
+ }
+ }
+ }
+ }
+ }
+ Ok(Arc::new(decimal_builder.finish()))
+}
+
+// parse the string format decimal value to i128 format.
+// like "125.12" to 12512_i128.
+fn parse_decimal(s: &str) -> Result<i128> {
+ if DECIMAL_RE.is_match(s) {
+ let mut offset = s.len();
+ // each byte is digit、'-' or '.'
+ let bytes = s.as_bytes();
+ let mut negitive = false;
+ let mut result: i128 = 0;
Review comment:
I think you could probably use `str::parse` here (after first removing all `'.'`) rather than a custom parsing loop
For example:
```rust
let i: i128 = "123".parse().unwrap();
println!("i is {}", i);
```
prints `"123"`
--
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 merged pull request #941: Support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941
--
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 #941: ARROW-RS-926: support read decimal data from csv reader if user provide the schema with decimal data type
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #941:
URL: https://github.com/apache/arrow-rs/pull/941#issuecomment-966226692
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/941?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 [#941](https://codecov.io/gh/apache/arrow-rs/pull/941?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0b25e97) into [master](https://codecov.io/gh/apache/arrow-rs/commit/1945f6f96551e501010a4578295456a1f9f335e9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1945f6f) will **increase** coverage by `0.01%`.
> The diff coverage is `87.69%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/941?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 #941 +/- ##
==========================================
+ Coverage 82.34% 82.35% +0.01%
==========================================
Files 168 168
Lines 48520 48606 +86
==========================================
+ Hits 39955 40031 +76
- Misses 8565 8575 +10
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/941?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/941/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.84% <87.69%> (-0.19%)` | :arrow_down: |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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==) | `65.95% <0.00%> (-0.43%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.52% <0.00%> (-0.20%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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=) | `85.33% <0.00%> (ø)` | |
| [arrow/src/compute/kernels/take.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy90YWtlLnJz) | `95.22% <0.00%> (+0.14%)` | :arrow_up: |
| [arrow/src/error.rs](https://codecov.io/gh/apache/arrow-rs/pull/941/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-YXJyb3cvc3JjL2Vycm9yLnJz) | `15.55% <0.00%> (+2.22%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/941?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/941?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 [1945f6f...0b25e97](https://codecov.io/gh/apache/arrow-rs/pull/941?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