Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Jefffrey merged PR #9936: URL: https://github.com/apache/arrow-rs/pull/9936 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Jefffrey commented on PR #9936: URL: https://github.com/apache/arrow-rs/pull/9936#issuecomment-4552469228 Thanks @Rich-T-kid -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Rich-T-kid commented on code in PR #9936:
URL: https://github.com/apache/arrow-rs/pull/9936#discussion_r3295899370
##
parquet/benches/arrow_writer.rs:
##
@@ -363,8 +384,10 @@ fn write_batch_with_option(
bench.iter(|| {
let mut file = Empty::default();
-let mut writer =
-ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone())).unwrap();
+let Ok(mut writer) = ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone()))
Review Comment:
this makes sense to me, commented out the benchmarks, and included a lint
bypass to pass CI , with a comment linking this discussion.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Jefffrey commented on code in PR #9936:
URL: https://github.com/apache/arrow-rs/pull/9936#discussion_r3295722019
##
parquet/benches/arrow_writer.rs:
##
@@ -363,8 +384,10 @@ fn write_batch_with_option(
bench.iter(|| {
let mut file = Empty::default();
-let mut writer =
-ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone())).unwrap();
+let Ok(mut writer) = ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone()))
Review Comment:
Generally its easier to have benchmarks merged to main since it lets us use
the bot to run the comparison; given we don't have a baseline anyway since its
not really supported, maybe we can just comment out the lines with the ree
benchmarks in this PR, e.g.
```rust
// let batch = create_ree_bench_batch(DataType::Utf8, BATCH_SIZE, 0.25,
0.75).unwrap();
// batches.push(("string_ree", batch));
// let batch = create_ree_bench_batch(DataType::Int32, BATCH_SIZE, 0.25,
0.75).unwrap();
// batches.push(("int32_ree", batch));
```
So we don't need to have this handling for potentially unsupported datatypes
in the benchmarks which can be confusing.
This way we can still have this benchmark code ready and the main PR later
won't get bogged down (other than uncommenting the lines)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Rich-T-kid commented on PR #9936: URL: https://github.com/apache/arrow-rs/pull/9936#issuecomment-4529219097 pushed up a revision 🚀 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Rich-T-kid commented on code in PR #9936:
URL: https://github.com/apache/arrow-rs/pull/9936#discussion_r3294900501
##
parquet/benches/arrow_writer.rs:
##
@@ -363,8 +384,10 @@ fn write_batch_with_option(
bench.iter(|| {
let mut file = Empty::default();
-let mut writer =
-ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone())).unwrap();
+let Ok(mut writer) = ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone()))
Review Comment:
This PR is to introduce benchmarks for
https://github.com/apache/arrow-rs/issues/8016 , so that we can [gauge the
performance](https://github.com/apache/arrow-rs/issues/8016#issuecomment-4390890656)
of changes as they happen. I thought I'd make sense to make it a separate PR
since it can be isolated from the write logic & would be easier to review.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Jefffrey commented on code in PR #9936:
URL: https://github.com/apache/arrow-rs/pull/9936#discussion_r3292032405
##
arrow/src/util/data_gen.rs:
##
@@ -230,6 +233,62 @@ fn create_random_decimal_array(field: &Field, size: usize,
null_density: f32) ->
))),
}
}
+#[inline]
+fn create_random_run_end_encoded_array(
+index: &Field,
+value: &Field,
+size: usize,
+null_density: f32,
+true_density: f32,
+) -> Result {
+const MIN_RUN: usize = 8;
Review Comment:
Probably fine as is for now
##
arrow/src/util/data_gen.rs:
##
@@ -648,6 +707,36 @@ mod tests {
assert_eq!(col_d_y.null_count(), 0);
}
+#[test]
+fn test_create_run_end_encoded_array() {
+let size = 1000;
+let ree_field = Field::new(
+"ree",
+DataType::RunEndEncoded(
+Arc::new(Field::new("run_ends", DataType::Int32, false)),
+Arc::new(Field::new("values", DataType::Utf8, true)),
+),
+false,
+);
+
+let array = create_random_array(&ree_field, size, 0.25, 0.0).unwrap();
+assert_eq!(array.len(), size);
+
+let ree = array
+.as_any()
+.downcast_ref::>()
+.unwrap();
+let run_ends = ree.run_ends().values();
+let num_runs = run_ends.len();
+
+for w in run_ends.windows(2) {
+assert!(w[0] < w[1]);
Review Comment:
nit: this is technically enforced during `RunArray::try_new` validation
anyway
##
parquet/benches/arrow_writer.rs:
##
@@ -363,8 +384,10 @@ fn write_batch_with_option(
bench.iter(|| {
let mut file = Empty::default();
-let mut writer =
-ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone())).unwrap();
+let Ok(mut writer) = ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone()))
Review Comment:
I don't follow; why do we need these changes in the first place?
##
arrow/src/util/data_gen.rs:
##
@@ -648,6 +707,36 @@ mod tests {
assert_eq!(col_d_y.null_count(), 0);
}
+#[test]
+fn test_create_run_end_encoded_array() {
+let size = 1000;
+let ree_field = Field::new(
+"ree",
+DataType::RunEndEncoded(
+Arc::new(Field::new("run_ends", DataType::Int32, false)),
+Arc::new(Field::new("values", DataType::Utf8, true)),
+),
+false,
+);
+
+let array = create_random_array(&ree_field, size, 0.25, 0.0).unwrap();
+assert_eq!(array.len(), size);
+
+let ree = array
+.as_any()
+.downcast_ref::>()
+.unwrap();
Review Comment:
```suggestion
let ree = array.as_run::();
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Rich-T-kid commented on PR #9936: URL: https://github.com/apache/arrow-rs/pull/9936#issuecomment-4523240447 @Jefffrey / @brancz when you get a chance could you take a look at this -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Rich-T-kid commented on code in PR #9936:
URL: https://github.com/apache/arrow-rs/pull/9936#discussion_r3243296413
##
parquet/benches/arrow_writer.rs:
##
@@ -401,6 +424,9 @@ fn create_batches() -> Vec<(&'static str, RecordBatch)> {
let batch = create_string_bench_batch_non_null(BATCH_SIZE, 0.25,
0.75).unwrap();
batches.push(("string_non_null", batch));
+let batch = create_string_ree_bench_batch(BATCH_SIZE, 0.25, 0.75).unwrap();
Review Comment:
maybe other types can be added? other primitive types such as [int,float] as
well as writing out nested types like [list,struct,ree over ree]
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Rich-T-kid commented on code in PR #9936:
URL: https://github.com/apache/arrow-rs/pull/9936#discussion_r3242936421
##
parquet/benches/arrow_writer.rs:
##
@@ -363,8 +384,10 @@ fn write_batch_with_option(
bench.iter(|| {
let mut file = Empty::default();
-let mut writer =
-ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone())).unwrap();
+let Ok(mut writer) = ArrowWriter::try_new(&mut file, batch.schema(),
Some(props.clone()))
Review Comment:
This approach adds no overhead for the regular (non-ree) branches since
unwrap should boil down to the same machine code as this expression. the issue
is this doesn't allow for errors to be propagated for other reasons. this
shouldnt be an issue for existing benchmarks since they all run fine with no
issues. is there another way to go about this?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] benchmarks for writing REE arrays to parquet [arrow-rs]
Rich-T-kid commented on code in PR #9936:
URL: https://github.com/apache/arrow-rs/pull/9936#discussion_r3242910355
##
arrow/src/util/data_gen.rs:
##
@@ -230,6 +233,62 @@ fn create_random_decimal_array(field: &Field, size: usize,
null_density: f32) ->
))),
}
}
+#[inline]
+fn create_random_run_end_encoded_array(
+index: &Field,
+value: &Field,
+size: usize,
+null_density: f32,
+true_density: f32,
+) -> Result {
+const MIN_RUN: usize = 8;
Review Comment:
Would it be worth it to add these as parameters or is it fine like this?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
