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/07/11 04:52:46 UTC
[GitHub] [arrow-rs] silathdiir opened a new pull request #537: Implement `RecordBatch::concat`
silathdiir opened a new pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537
# Which issue does this PR close?
Closes #461 .
# Rationale for this change
As described in the issue, tries to implement `RecordBatch::concat` according to https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/physical_plan/coalesce_batches.rs#L232 .
# What changes are included in this PR?
Adds a new function `concat` to struct `RecordBatch`, and test cases.
# Are there any user-facing changes?
With this fix, a new `RecordBatch` could be created by concatenating multiple `RecordBatch`es.
--
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 #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#issuecomment-879825878
Thanks again @silathdiir !
--
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 #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537
--
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 #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#discussion_r667467518
##########
File path: arrow/src/record_batch.rs
##########
@@ -639,4 +669,108 @@ mod tests {
assert_eq!(batch.column(0).as_ref(), boolean.as_ref());
assert_eq!(batch.column(1).as_ref(), int.as_ref());
}
+
+ #[test]
+ fn concat_empty_record_batches() {
+ let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
+ let batch = RecordBatch::concat(&schema, &[]).unwrap();
+ assert_eq!(batch.schema().as_ref(), schema.as_ref());
+ assert_eq!(0, batch.num_rows());
+ }
+
+ #[test]
+ fn concat_record_batches_of_same_schema() {
+ let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
+ let batch1 = RecordBatch::try_new(
+ schema.clone(),
+ vec![Arc::new(Int32Array::from(vec![1, 2]))],
+ )
+ .unwrap();
+ let batch2 = RecordBatch::try_new(
+ schema.clone(),
+ vec![Arc::new(Int32Array::from(vec![3, 4]))],
+ )
+ .unwrap();
+ let new_batch = RecordBatch::concat(&schema, &[batch1, batch2]).unwrap();
+ assert_eq!(new_batch.schema().as_ref(), schema.as_ref());
+ assert_eq!(4, new_batch.num_rows());
+ }
+
+ #[test]
+ fn concat_record_batches_of_similar_schemas() {
Review comment:
I would suggest this case be treated as an error -- if a user wants this behavior I think they should explicitly have to specifically pick out the columns they want and create record batches with the same schema
##########
File path: arrow/src/record_batch.rs
##########
@@ -353,6 +354,35 @@ impl RecordBatch {
let schema = Arc::new(Schema::new(fields));
RecordBatch::try_new(schema, columns)
}
+
+ /// Concatenates `batches` together into a single record batch.
+ pub fn concat(schema: &SchemaRef, batches: &[Self]) -> Result<Self> {
+ if batches.is_empty() {
+ return Ok(RecordBatch::new_empty(schema.clone()));
+ }
+ let field_num = schema.fields().len();
+ if let Some((i, _)) = batches
+ .iter()
+ .enumerate()
+ .find(|&(_, batch)| batch.num_columns() < field_num)
Review comment:
I think if the batches don't have the same schema, an error should result, even if there is some plausible behavior.
My rationale is that it is likely a user error if the schemas don't match, so failing fast will help the programmer identify the problem more easily
So perhaps this function should check `batch.schema() != schema` (aka compare the entire schema for each record batch)
##########
File path: arrow/src/record_batch.rs
##########
@@ -639,4 +669,108 @@ mod tests {
assert_eq!(batch.column(0).as_ref(), boolean.as_ref());
assert_eq!(batch.column(1).as_ref(), int.as_ref());
}
+
+ #[test]
+ fn concat_empty_record_batches() {
+ let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
+ let batch = RecordBatch::concat(&schema, &[]).unwrap();
+ assert_eq!(batch.schema().as_ref(), schema.as_ref());
+ assert_eq!(0, batch.num_rows());
+ }
+
+ #[test]
+ fn concat_record_batches_of_same_schema() {
+ let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
+ let batch1 = RecordBatch::try_new(
+ schema.clone(),
+ vec![Arc::new(Int32Array::from(vec![1, 2]))],
+ )
+ .unwrap();
+ let batch2 = RecordBatch::try_new(
+ schema.clone(),
+ vec![Arc::new(Int32Array::from(vec![3, 4]))],
+ )
+ .unwrap();
+ let new_batch = RecordBatch::concat(&schema, &[batch1, batch2]).unwrap();
+ assert_eq!(new_batch.schema().as_ref(), schema.as_ref());
+ assert_eq!(4, new_batch.num_rows());
+ }
+
+ #[test]
+ fn concat_record_batches_of_similar_schemas() {
+ let schema1 =
+ Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
+ let schema2 = Arc::new(Schema::new(vec![
+ Field::new("b", DataType::Int32, false),
+ Field::new("c", DataType::Utf8, false),
+ ]));
+ let batch1 = RecordBatch::try_new(
+ schema1.clone(),
+ vec![Arc::new(Int32Array::from(vec![1, 2]))],
+ )
+ .unwrap();
+ let batch2 = RecordBatch::try_new(
+ schema2,
+ vec![
+ Arc::new(Int32Array::from(vec![3, 4])),
+ Arc::new(StringArray::from(vec!["a", "b"])),
+ ],
+ )
+ .unwrap();
+ let new_batch = RecordBatch::concat(&schema1, &[batch1, batch2]).unwrap();
+ assert_eq!(new_batch.schema().as_ref(), schema1.as_ref());
+ assert_eq!(4, new_batch.num_rows());
+ }
+
+ #[test]
+ fn concat_record_batches_of_different_column_num() {
+ let schema1 = Arc::new(Schema::new(vec![
+ Field::new("a", DataType::Int32, false),
+ Field::new("b", DataType::Utf8, false),
+ ]));
+ let schema2 =
+ Arc::new(Schema::new(vec![Field::new("c", DataType::Int32, false)]));
+ let batch1 = RecordBatch::try_new(
+ schema1.clone(),
+ vec![
+ Arc::new(Int32Array::from(vec![1, 2])),
+ Arc::new(StringArray::from(vec!["a", "b"])),
+ ],
+ )
+ .unwrap();
+ let batch2 =
+ RecordBatch::try_new(schema2, vec![Arc::new(Int32Array::from(vec![3, 4]))])
+ .unwrap();
+ match RecordBatch::concat(&schema1, &[batch1, batch2]) {
+ Err(ArrowError::InvalidArgumentError(s)) => {
+ assert_eq!(
+ "batches[1] has insufficient number of columns as 2 fields in schema.", s)
+ }
+ _ => panic!("should not be other result"),
+ }
Review comment:
Another pattern to check errors I have seen is to use `unwrap_err()`, something like
```suggestion
let error = RecordBatch::concat(&schema1, &[batch1, batch2]).unwrap_err();
assert_eq(error.to_string(), "batches[1] has insufficient number of columns as 2 fields in schema.")
```
This one is fine too, however but I figured I would mention it.
##########
File path: arrow/src/record_batch.rs
##########
@@ -639,4 +669,108 @@ mod tests {
assert_eq!(batch.column(0).as_ref(), boolean.as_ref());
assert_eq!(batch.column(1).as_ref(), int.as_ref());
}
+
+ #[test]
+ fn concat_empty_record_batches() {
+ let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
+ let batch = RecordBatch::concat(&schema, &[]).unwrap();
+ assert_eq!(batch.schema().as_ref(), schema.as_ref());
+ assert_eq!(0, batch.num_rows());
+ }
+
+ #[test]
+ fn concat_record_batches_of_same_schema() {
+ let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
Review comment:
I suggest making the positive case have 2 columns rather than just a single one to get coverage on the loops
--
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] silathdiir edited a comment on pull request #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
silathdiir edited a comment on pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#issuecomment-877912374
Hi @alamb, refer to @jorgecarleitao latest comment [#461 (comment)](https://github.com/apache/arrow-rs/issues/461#issuecomment-877808997), it seems that `arrow::compute::concat::concat_batches` may be better (even seems not good enough for thread model).
I will follow the discussion and try to update this PR.
--
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] silathdiir commented on a change in pull request #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
silathdiir commented on a change in pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#discussion_r667422194
##########
File path: arrow/src/record_batch.rs
##########
@@ -353,6 +354,35 @@ impl RecordBatch {
let schema = Arc::new(Schema::new(fields));
RecordBatch::try_new(schema, columns)
}
+
+ /// Concatenates `batches` together into a single record batch.
+ pub fn concat(schema: &SchemaRef, batches: &[Self]) -> Result<Self> {
+ if batches.is_empty() {
+ return Ok(RecordBatch::new_empty(schema.clone()));
+ }
+ let field_num = schema.fields().len();
+ if let Some((i, _)) = batches
+ .iter()
+ .enumerate()
+ .find(|&(_, batch)| batch.num_columns() < field_num)
Review comment:
I could not confirm if a batch of more columns than schema could be allowed to be concatenated, as below test case [concat_record_batches_of_similar_schemas](https://github.com/apache/arrow-rs/pull/537/files#diff-9841f61221a67c626eb60a49f2aeb7de6c1a16ab478bc7cb2675f5807b32b8e6R700).
--
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 #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#issuecomment-877742354
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/537?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 [#537](https://codecov.io/gh/apache/arrow-rs/pull/537?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3a14809) into [master](https://codecov.io/gh/apache/arrow-rs/commit/f1fb2b11bbd6350365de010d3e1d676a27602d3a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f1fb2b1) will **increase** coverage by `0.01%`.
> The diff coverage is `93.10%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/537?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 #537 +/- ##
==========================================
+ Coverage 82.60% 82.62% +0.01%
==========================================
Files 167 167
Lines 45984 46042 +58
==========================================
+ Hits 37984 38041 +57
- Misses 8000 8001 +1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/537?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/record\_batch.rs](https://codecov.io/gh/apache/arrow-rs/pull/537/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-YXJyb3cvc3JjL3JlY29yZF9iYXRjaC5ycw==) | `90.44% <93.10%> (+1.18%)` | :arrow_up: |
| [arrow/src/error.rs](https://codecov.io/gh/apache/arrow-rs/pull/537/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) | `13.33% <0.00%> (+4.44%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/537?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/537?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 [f1fb2b1...3a14809](https://codecov.io/gh/apache/arrow-rs/pull/537?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] codecov-commenter commented on pull request #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#issuecomment-877742354
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/537?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 [#537](https://codecov.io/gh/apache/arrow-rs/pull/537?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e1f24d1) into [master](https://codecov.io/gh/apache/arrow-rs/commit/f1fb2b11bbd6350365de010d3e1d676a27602d3a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f1fb2b1) will **increase** coverage by `0.01%`.
> The diff coverage is `91.42%`.
> :exclamation: Current head e1f24d1 differs from pull request most recent head eec5c90. Consider uploading reports for the commit eec5c90 to get more accurate results
[](https://codecov.io/gh/apache/arrow-rs/pull/537?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 #537 +/- ##
==========================================
+ Coverage 82.60% 82.61% +0.01%
==========================================
Files 167 167
Lines 45984 46054 +70
==========================================
+ Hits 37984 38048 +64
- Misses 8000 8006 +6
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/537?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/record\_batch.rs](https://codecov.io/gh/apache/arrow-rs/pull/537/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-YXJyb3cvc3JjL3JlY29yZF9iYXRjaC5ycw==) | `90.14% <91.42%> (+0.88%)` | :arrow_up: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/537/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) | `94.85% <0.00%> (-0.20%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/537?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/537?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 [f1fb2b1...eec5c90](https://codecov.io/gh/apache/arrow-rs/pull/537?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 pull request #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#issuecomment-877785281
Although @nevi-me seems to like `RecordBatch::concat` -- https://github.com/apache/arrow-rs/issues/461#issuecomment-877784891
--
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 #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#issuecomment-879355147
Unless I hear different, I plan to merge this PR tomorrow (and include it in 5.0.0)
--
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] silathdiir commented on pull request #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
silathdiir commented on pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#issuecomment-877912374
Hi @alamb, refer to @jorgecarleitao latest comment [#461 (comment)](https://github.com/apache/arrow-rs/issues/461#issuecomment-877808997), it seems that `arrow::compute::concat::concat_batches` may be better (even not good enough for thread model).
I will follow the discussion and try to update this PR.
--
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 #537: Implement `RecordBatch::concat`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #537:
URL: https://github.com/apache/arrow-rs/pull/537#issuecomment-878361494
thanks @silathdiir !
--
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