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/10/12 19:12:18 UTC
[GitHub] [arrow-rs] nevi-me opened a new pull request #826: JSON reader - empty nested list should not create child value
nevi-me opened a new pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826
# Which issue does this PR close?
Closes #825.
# Rationale for this change
See issue for more context, this affects the ability to write nested data that has `<list<struct<_>>` if some lists are empty.
# What changes are included in this PR?
Fixed the reader to account for empty lists (`serde_json::Value::List(vec![])`) and not create an empty struct.
# Are there any user-facing changes?
No
--
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] nevi-me commented on a change in pull request #826: JSON reader - empty nested list should not create child value
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826#discussion_r727789375
##########
File path: arrow/src/json/reader.rs
##########
@@ -1048,31 +1048,28 @@ impl Decoder {
}
DataType::Struct(fields) => {
// extract list values, with non-lists converted to Value::Null
- let array_item_count = rows
- .iter()
- .map(|row| match row {
- Value::Array(values) => values.len(),
- _ => 1,
- })
- .sum();
+ let array_item_count = cur_offset.to_usize().unwrap();
let num_bytes = bit_util::ceil(array_item_count, 8);
let mut null_buffer = MutableBuffer::from_len_zeroed(num_bytes);
let mut struct_index = 0;
let rows: Vec<Value> = rows
.iter()
- .flat_map(|row| {
- if let Value::Array(values) = row {
- values.iter().for_each(|_| {
- bit_util::set_bit(
- null_buffer.as_slice_mut(),
- struct_index,
- );
+ .flat_map(|row| match row {
+ Value::Array(values) if !values.is_empty() => {
+ values.iter().for_each(|value| {
+ if let Value::Null = value {
Review comment:
`is_null()` does more, but the compiler can optimise it https://godbolt.org/z/76x4316Y9
So I'll change to that, thanks @houqp
--
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] houqp commented on a change in pull request #826: JSON reader - empty nested list should not create child value
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826#discussion_r727720505
##########
File path: arrow/src/json/reader.rs
##########
@@ -1048,31 +1048,28 @@ impl Decoder {
}
DataType::Struct(fields) => {
// extract list values, with non-lists converted to Value::Null
- let array_item_count = rows
- .iter()
- .map(|row| match row {
- Value::Array(values) => values.len(),
- _ => 1,
- })
- .sum();
+ let array_item_count = cur_offset.to_usize().unwrap();
let num_bytes = bit_util::ceil(array_item_count, 8);
let mut null_buffer = MutableBuffer::from_len_zeroed(num_bytes);
let mut struct_index = 0;
let rows: Vec<Value> = rows
.iter()
- .flat_map(|row| {
- if let Value::Array(values) = row {
- values.iter().for_each(|_| {
- bit_util::set_bit(
- null_buffer.as_slice_mut(),
- struct_index,
- );
+ .flat_map(|row| match row {
+ Value::Array(values) if !values.is_empty() => {
+ values.iter().for_each(|value| {
+ if let Value::Null = value {
Review comment:
how about using this method? https://docs.serde.rs/serde_json/enum.Value.html#method.is_null
--
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] nevi-me merged pull request #826: JSON reader - empty nested list should not create child value
Posted by GitBox <gi...@apache.org>.
nevi-me merged pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826
--
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 #826: JSON reader - empty nested list should not create child value
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826#issuecomment-941379392
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/826?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 [#826](https://codecov.io/gh/apache/arrow-rs/pull/826?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (740bb33) into [master](https://codecov.io/gh/apache/arrow-rs/commit/ed5b398a0c4574dec96e608659161c37aef18bb4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ed5b398) will **increase** coverage by `0.00%`.
> The diff coverage is `96.87%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/826?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 #826 +/- ##
=======================================
Coverage 82.57% 82.58%
=======================================
Files 168 168
Lines 47939 47955 +16
=======================================
+ Hits 39587 39602 +15
- Misses 8352 8353 +1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/826?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/json/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/826/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.74% <92.85%> (-0.09%)` | :arrow_down: |
| [arrow/src/json/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/826/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-YXJyb3cvc3JjL2pzb24vd3JpdGVyLnJz) | `92.24% <100.00%> (+0.31%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/826?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/826?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 [ed5b398...740bb33](https://codecov.io/gh/apache/arrow-rs/pull/826?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] nevi-me commented on a change in pull request #826: JSON reader - empty nested list should not create child value
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826#discussion_r727419966
##########
File path: arrow/src/json/reader.rs
##########
@@ -1048,31 +1048,28 @@ impl Decoder {
}
DataType::Struct(fields) => {
// extract list values, with non-lists converted to Value::Null
- let array_item_count = rows
- .iter()
- .map(|row| match row {
- Value::Array(values) => values.len(),
- _ => 1,
- })
- .sum();
+ let array_item_count = cur_offset.to_usize().unwrap();
let num_bytes = bit_util::ceil(array_item_count, 8);
let mut null_buffer = MutableBuffer::from_len_zeroed(num_bytes);
let mut struct_index = 0;
let rows: Vec<Value> = rows
.iter()
- .flat_map(|row| {
- if let Value::Array(values) = row {
- values.iter().for_each(|_| {
- bit_util::set_bit(
- null_buffer.as_slice_mut(),
- struct_index,
- );
+ .flat_map(|row| match row {
+ Value::Array(values) if !values.is_empty() => {
+ values.iter().for_each(|value| {
+ if let Value::Null = value {
Review comment:
I don't like this, but I'm saying "if value is not null" while avoiding a `match` statement
##########
File path: arrow/src/json/reader.rs
##########
@@ -1048,31 +1048,28 @@ impl Decoder {
}
DataType::Struct(fields) => {
// extract list values, with non-lists converted to Value::Null
- let array_item_count = rows
- .iter()
- .map(|row| match row {
- Value::Array(values) => values.len(),
- _ => 1,
- })
- .sum();
+ let array_item_count = cur_offset.to_usize().unwrap();
let num_bytes = bit_util::ceil(array_item_count, 8);
let mut null_buffer = MutableBuffer::from_len_zeroed(num_bytes);
let mut struct_index = 0;
let rows: Vec<Value> = rows
.iter()
- .flat_map(|row| {
- if let Value::Array(values) = row {
- values.iter().for_each(|_| {
- bit_util::set_bit(
- null_buffer.as_slice_mut(),
- struct_index,
- );
+ .flat_map(|row| match row {
+ Value::Array(values) if !values.is_empty() => {
+ values.iter().for_each(|value| {
+ if let Value::Null = value {
+ } else {
+ bit_util::set_bit(
+ null_buffer.as_slice_mut(),
+ struct_index,
+ );
+ }
struct_index += 1;
});
values.clone()
- } else {
- struct_index += 1;
- vec![Value::Null]
Review comment:
this is the crux of the issue
--
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] nevi-me commented on a change in pull request #826: JSON reader - empty nested list should not create child value
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826#discussion_r727419966
##########
File path: arrow/src/json/reader.rs
##########
@@ -1048,31 +1048,28 @@ impl Decoder {
}
DataType::Struct(fields) => {
// extract list values, with non-lists converted to Value::Null
- let array_item_count = rows
- .iter()
- .map(|row| match row {
- Value::Array(values) => values.len(),
- _ => 1,
- })
- .sum();
+ let array_item_count = cur_offset.to_usize().unwrap();
let num_bytes = bit_util::ceil(array_item_count, 8);
let mut null_buffer = MutableBuffer::from_len_zeroed(num_bytes);
let mut struct_index = 0;
let rows: Vec<Value> = rows
.iter()
- .flat_map(|row| {
- if let Value::Array(values) = row {
- values.iter().for_each(|_| {
- bit_util::set_bit(
- null_buffer.as_slice_mut(),
- struct_index,
- );
+ .flat_map(|row| match row {
+ Value::Array(values) if !values.is_empty() => {
+ values.iter().for_each(|value| {
+ if let Value::Null = value {
Review comment:
I don't like this, but I'm saying "if value is not null" while avoiding a `match` statement
##########
File path: arrow/src/json/reader.rs
##########
@@ -1048,31 +1048,28 @@ impl Decoder {
}
DataType::Struct(fields) => {
// extract list values, with non-lists converted to Value::Null
- let array_item_count = rows
- .iter()
- .map(|row| match row {
- Value::Array(values) => values.len(),
- _ => 1,
- })
- .sum();
+ let array_item_count = cur_offset.to_usize().unwrap();
let num_bytes = bit_util::ceil(array_item_count, 8);
let mut null_buffer = MutableBuffer::from_len_zeroed(num_bytes);
let mut struct_index = 0;
let rows: Vec<Value> = rows
.iter()
- .flat_map(|row| {
- if let Value::Array(values) = row {
- values.iter().for_each(|_| {
- bit_util::set_bit(
- null_buffer.as_slice_mut(),
- struct_index,
- );
+ .flat_map(|row| match row {
+ Value::Array(values) if !values.is_empty() => {
+ values.iter().for_each(|value| {
+ if let Value::Null = value {
+ } else {
+ bit_util::set_bit(
+ null_buffer.as_slice_mut(),
+ struct_index,
+ );
+ }
struct_index += 1;
});
values.clone()
- } else {
- struct_index += 1;
- vec![Value::Null]
Review comment:
this is the crux of the issue
##########
File path: arrow/src/json/reader.rs
##########
@@ -1048,31 +1048,28 @@ impl Decoder {
}
DataType::Struct(fields) => {
// extract list values, with non-lists converted to Value::Null
- let array_item_count = rows
- .iter()
- .map(|row| match row {
- Value::Array(values) => values.len(),
- _ => 1,
- })
- .sum();
+ let array_item_count = cur_offset.to_usize().unwrap();
let num_bytes = bit_util::ceil(array_item_count, 8);
let mut null_buffer = MutableBuffer::from_len_zeroed(num_bytes);
let mut struct_index = 0;
let rows: Vec<Value> = rows
.iter()
- .flat_map(|row| {
- if let Value::Array(values) = row {
- values.iter().for_each(|_| {
- bit_util::set_bit(
- null_buffer.as_slice_mut(),
- struct_index,
- );
+ .flat_map(|row| match row {
+ Value::Array(values) if !values.is_empty() => {
+ values.iter().for_each(|value| {
+ if let Value::Null = value {
Review comment:
`is_null()` does more, but the compiler can optimise it https://godbolt.org/z/76x4316Y9
So I'll change to that, thanks @houqp
--
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] nevi-me merged pull request #826: JSON reader - empty nested list should not create child value
Posted by GitBox <gi...@apache.org>.
nevi-me merged pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826
--
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 #826: JSON reader - empty nested list should not create child value
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826#issuecomment-941379392
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/826?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 [#826](https://codecov.io/gh/apache/arrow-rs/pull/826?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (740bb33) into [master](https://codecov.io/gh/apache/arrow-rs/commit/ed5b398a0c4574dec96e608659161c37aef18bb4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ed5b398) will **increase** coverage by `0.00%`.
> The diff coverage is `96.87%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/826?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 #826 +/- ##
=======================================
Coverage 82.57% 82.58%
=======================================
Files 168 168
Lines 47939 47955 +16
=======================================
+ Hits 39587 39602 +15
- Misses 8352 8353 +1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/826?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/json/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/826/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.74% <92.85%> (-0.09%)` | :arrow_down: |
| [arrow/src/json/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/826/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-YXJyb3cvc3JjL2pzb24vd3JpdGVyLnJz) | `92.24% <100.00%> (+0.31%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/826?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/826?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 [ed5b398...740bb33](https://codecov.io/gh/apache/arrow-rs/pull/826?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] houqp commented on a change in pull request #826: JSON reader - empty nested list should not create child value
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #826:
URL: https://github.com/apache/arrow-rs/pull/826#discussion_r727720505
##########
File path: arrow/src/json/reader.rs
##########
@@ -1048,31 +1048,28 @@ impl Decoder {
}
DataType::Struct(fields) => {
// extract list values, with non-lists converted to Value::Null
- let array_item_count = rows
- .iter()
- .map(|row| match row {
- Value::Array(values) => values.len(),
- _ => 1,
- })
- .sum();
+ let array_item_count = cur_offset.to_usize().unwrap();
let num_bytes = bit_util::ceil(array_item_count, 8);
let mut null_buffer = MutableBuffer::from_len_zeroed(num_bytes);
let mut struct_index = 0;
let rows: Vec<Value> = rows
.iter()
- .flat_map(|row| {
- if let Value::Array(values) = row {
- values.iter().for_each(|_| {
- bit_util::set_bit(
- null_buffer.as_slice_mut(),
- struct_index,
- );
+ .flat_map(|row| match row {
+ Value::Array(values) if !values.is_empty() => {
+ values.iter().for_each(|value| {
+ if let Value::Null = value {
Review comment:
how about using this method? https://docs.serde.rs/serde_json/enum.Value.html#method.is_null
--
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