You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "dadepo (via GitHub)" <gi...@apache.org> on 2023/06/05 10:50:02 UTC
[GitHub] [arrow-rs] dadepo opened a new pull request, #4364: Have array_to_json_array support MapArray
dadepo opened a new pull request, #4364:
URL: https://github.com/apache/arrow-rs/pull/4364
# Which issue does this PR close?
Closes #4297
# Rationale for this change
Extended `array_to_json_array` to support `MapArray`
--
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] tustvold commented on a diff in pull request #4364: Have array_to_json_array support MapArray
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4364:
URL: https://github.com/apache/arrow-rs/pull/4364#discussion_r1217894253
##########
arrow-array/src/array/map_array.rs:
##########
@@ -284,6 +292,18 @@ impl Array for MapArray {
}
}
+impl<'a> ArrayAccessor for &'a MapArray {
+ type Item = ArrayRef;
Review Comment:
`StructArray` should be strictly more useful?
--
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] dadepo commented on a diff in pull request #4364: Have array_to_json_array support MapArray
Posted by "dadepo (via GitHub)" <gi...@apache.org>.
dadepo commented on code in PR #4364:
URL: https://github.com/apache/arrow-rs/pull/4364#discussion_r1217900578
##########
arrow-array/src/array/map_array.rs:
##########
@@ -284,6 +292,18 @@ impl Array for MapArray {
}
}
+impl<'a> ArrayAccessor for &'a MapArray {
+ type Item = ArrayRef;
Review Comment:
Yeah, just thinking without that, the consumer will always be forced to cast back to `StructArray`.
I'll update it in a bit
##########
arrow-array/src/array/map_array.rs:
##########
@@ -284,6 +292,18 @@ impl Array for MapArray {
}
}
+impl<'a> ArrayAccessor for &'a MapArray {
+ type Item = ArrayRef;
Review Comment:
Yeah, just thinking, without that, the consumer will always be forced to cast back to `StructArray`.
I'll update it in a bit
--
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] tustvold merged pull request #4364: Have array_to_json_array support MapArray
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #4364:
URL: https://github.com/apache/arrow-rs/pull/4364
--
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] tustvold commented on a diff in pull request #4364: Have array_to_json_array support MapArray
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4364:
URL: https://github.com/apache/arrow-rs/pull/4364#discussion_r1218317323
##########
arrow-json/src/writer.rs:
##########
@@ -202,6 +203,15 @@ pub fn array_to_json_array(array: &ArrayRef) -> Result<Vec<Value>, ArrowError> {
let jsonmaps = struct_array_to_jsonmap_array(array.as_struct())?;
Ok(jsonmaps.into_iter().map(Value::Object).collect())
}
+ DataType::Map(_, _) => as_map_array(array)
+ .iter()
+ .map(|maybe_value| match maybe_value {
+ Some(v) => Ok(Value::Array(array_to_json_array(
Review Comment:
We should probably change array_to_json_array to take `&dyn Array` to avoid the needless Arc-wrapping
--
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] dadepo commented on a diff in pull request #4364: Have array_to_json_array support MapArray
Posted by "dadepo (via GitHub)" <gi...@apache.org>.
dadepo commented on code in PR #4364:
URL: https://github.com/apache/arrow-rs/pull/4364#discussion_r1217893321
##########
arrow-array/src/array/map_array.rs:
##########
@@ -284,6 +292,18 @@ impl Array for MapArray {
}
}
+impl<'a> ArrayAccessor for &'a MapArray {
+ type Item = ArrayRef;
Review Comment:
@tustvold any thoughts on having the iterator return `ArrayRef` instead of `StructArray`?
--
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