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 2022/07/30 05:27:05 UTC
[GitHub] [arrow-rs] Ted-Jiang opened a new pull request, #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Ted-Jiang opened a new pull request, #2237:
URL: https://github.com/apache/arrow-rs/pull/2237
# Which issue does this PR close?
Closes #2236.
Related #2197
# Rationale for this change
Separate add read_records and consume_batch in `ArrayReader`, so we can read_records multi times in buffer and consume once (avoid small batch in skipping read).
# 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] tustvold commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933983922
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -135,23 +135,44 @@ where
.iter_mut()
.for_each(|buf| buf.truncate(num_read));
- self.def_levels_buffer = def_levels_buffer;
- self.rep_levels_buffer = rep_levels_buffer;
+ if let Some(mut def_levels_buffer) = def_levels_buffer {
+ match &mut self.def_levels_buffer {
+ None => {
+ self.def_levels_buffer = Some(def_levels_buffer);
+ }
+ Some(buf) => buf.append(&mut def_levels_buffer),
+ }
+ }
+
+ if let Some(mut rep_levels_buffer) = rep_levels_buffer {
+ match &mut self.rep_levels_buffer {
+ None => {
+ self.rep_levels_buffer = Some(rep_levels_buffer);
+ }
+ Some(buf) => buf.append(&mut rep_levels_buffer),
+ }
+ }
+
+ self.data_buffer.append(&mut data_buffer);
+ Ok(num_read)
+ }
+
+ fn consume_batch(&mut self) -> Result<ArrayRef> {
let data: Vec<Option<T::T>> = if self.def_levels_buffer.is_some() {
- data_buffer
- .into_iter()
+ self.data_buffer
Review Comment:
Perhaps we could use std::mem::take to avoid a copy here?
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +181,10 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = vec![];
+ self.def_levels_buffer = None;
+ self.rep_levels_buffer = None;
Review Comment:
I think this will break, as RecordReader assumes the definition levels live until the next call to consume_batch?
I'm not sure we actually have test coverage of say a nullable StructArray containing a DecimalArray π€
--
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] Ted-Jiang commented on pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#issuecomment-1200424282
> ComplexObjectArrayReader
I am
> I think there is a still an issue with ComplexObjectArrayReader. I think it needs to keep the levels data around for longer to avoid breaking parent ArrayReader. Otherwise looking very nice π
>
> Edit: I think ComplexObjectArrayReader might just be broken, I'll experiment when I get to a computer, and potentially accelerate plans to just remove it
Yep, it's quit complicated.π€
--
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] ursabot commented on pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#issuecomment-1203608744
Benchmark runs are scheduled for baseline = 6b2c757fcca57d3380b6640b3aa6cb97757ab3f0 and contender = 1f9973cd6cdde73883da50d9417126390437ef76. 1f9973cd6cdde73883da50d9417126390437ef76 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/9588c7562da74b98bfd47c0cf1d22864...dde7ff3ddeb9415aa37d3e6c34584084/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/dc309d602f97434cae899817094d1d86...7becfc75c5414f3880e3ee2cd2913a7d/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/52a3a3eddc2f4c5a9bfbea1d2a2991ff...e3efa608528e41298739b3da2ee277d1/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/3794047c458b4a0c9fd53ee7d350cec8...c8e80424285141ec88eb49fe4c8d4520/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933755699
##########
parquet/src/arrow/array_reader/null_array.rs:
##########
@@ -80,8 +80,15 @@ where
/// Reads at most `batch_size` records into array.
fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
- read_records(&mut self.record_reader, self.pages.as_mut(), batch_size)?;
+ let size = self.read_records(batch_size)?;
Review Comment:
IMO, i think we can keep `next_batch` , it just use `read_records` and `consume_batch` in order.
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r935690653
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +181,10 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = vec![];
+ self.def_levels_buffer = None;
+ self.rep_levels_buffer = None;
Review Comment:
@tustvold Sorry for the late reply, fix at [fix new ut](https://github.com/apache/arrow-rs/pull/2237/commits/429e2413f7d6f19bc5f627592e3c233018dac4cf)
--
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 #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r935850997
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -43,6 +43,9 @@ where
column_desc: ColumnDescPtr,
column_reader: Option<ColumnReaderImpl<T>>,
converter: C,
+ need_consume_def_levels_buffer: Option<Vec<i16>>,
Review Comment:
I'd call this something like `in_progress_def_levels`
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -206,11 +213,19 @@ where
}
fn get_def_levels(&self) -> Option<&[i16]> {
- self.def_levels_buffer.as_deref()
+ if self.before_consume {
Review Comment:
I think this can just return `self.def_levels_buffer.as_deref()`, if you look at PrimitiveArrayReader it will only make the data available after the call to `consume_batch`
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933961394
##########
parquet/src/arrow/array_reader/byte_array.rs:
##########
@@ -109,7 +109,23 @@ impl<I: OffsetSizeTrait + ScalarValue> ArrayReader for ByteArrayReader<I> {
}
fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
- read_records(&mut self.record_reader, self.pages.as_mut(), batch_size)?;
+ let size = self.read_records(batch_size)?;
Review Comment:
Yes.
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933988497
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -135,23 +135,44 @@ where
.iter_mut()
.for_each(|buf| buf.truncate(num_read));
- self.def_levels_buffer = def_levels_buffer;
- self.rep_levels_buffer = rep_levels_buffer;
+ if let Some(mut def_levels_buffer) = def_levels_buffer {
+ match &mut self.def_levels_buffer {
+ None => {
+ self.def_levels_buffer = Some(def_levels_buffer);
+ }
+ Some(buf) => buf.append(&mut def_levels_buffer),
+ }
+ }
+
+ if let Some(mut rep_levels_buffer) = rep_levels_buffer {
+ match &mut self.rep_levels_buffer {
+ None => {
+ self.rep_levels_buffer = Some(rep_levels_buffer);
+ }
+ Some(buf) => buf.append(&mut rep_levels_buffer),
+ }
+ }
+
+ self.data_buffer.append(&mut data_buffer);
+ Ok(num_read)
+ }
+
+ fn consume_batch(&mut self) -> Result<ArrayRef> {
let data: Vec<Option<T::T>> = if self.def_levels_buffer.is_some() {
- data_buffer
- .into_iter()
+ self.data_buffer
Review Comment:
Thanks π forgot this API, will keep in mind πͺ
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933987546
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +181,10 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = vec![];
+ self.def_levels_buffer = None;
+ self.rep_levels_buffer = None;
Review Comment:
you mean structArray which contains one child use `complex_object_array` reader? π€ why need the child re?
--
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 #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933991080
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +181,10 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = vec![];
+ self.def_levels_buffer = None;
+ self.rep_levels_buffer = None;
Review Comment:
If StructArray reader is itself nullable it needs to read the definition levels read by its child in order to work out where it's NULLs are located. It's a similar story for ListArrayReader.
I'll try to rustle up some tests this evening, so that we can be confident this PR won't break anything. Longer term I want to remove ComplexObjectArrayReader as it is slow, complicated and largely replaced by the newer generics.
Edit: ran out of time today, will look into first thing tomorrow
--
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 #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933810032
##########
parquet/src/arrow/array_reader/empty_array.rs:
##########
@@ -54,14 +54,21 @@ impl ArrayReader for EmptyArrayReader {
}
fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
+ let size = self.read_records(batch_size)?;
+ self.consume_batch(size)
+ }
+
+ fn read_records(&mut self, batch_size: usize) -> Result<usize> {
let len = self.remaining_rows.min(batch_size);
self.remaining_rows -= len;
+ Ok(len)
+ }
+ fn consume_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
let data = ArrayDataBuilder::new(self.data_type.clone())
- .len(len)
+ .len(batch_size)
Review Comment:
I think this needs to keep track of how many values have actually been read in read_records. Otherwise I think this will keep yielding values beyond the actual length of the column :sweat_smile:
##########
parquet/src/arrow/array_reader/mod.rs:
##########
@@ -115,7 +125,7 @@ impl RowGroupCollection for Arc<dyn FileReader> {
///
/// Returns the number of records read, which can be less than `batch_size` if
/// pages is exhausted.
-fn read_records<V, CV>(
+fn read_records_inner<V, CV>(
Review Comment:
Why rename this? I think this is now even less clear, as the concept of `inner` for a free function is a bit... undefined :smile:
##########
parquet/src/arrow/array_reader/byte_array.rs:
##########
@@ -109,7 +109,23 @@ impl<I: OffsetSizeTrait + ScalarValue> ArrayReader for ByteArrayReader<I> {
}
fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
- read_records(&mut self.record_reader, self.pages.as_mut(), batch_size)?;
+ let size = self.read_records(batch_size)?;
+ self.consume_batch(size)
+ }
+
+ fn read_records(&mut self, batch_size: usize) -> Result<usize> {
+ read_records_inner(&mut self.record_reader, self.pages.as_mut(), batch_size)
+ }
+
+ fn consume_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
Review Comment:
I think it would be simpler to remove the `batch_size` parameter and just return what has been buffered
##########
parquet/src/arrow/array_reader/byte_array.rs:
##########
@@ -109,7 +109,23 @@ impl<I: OffsetSizeTrait + ScalarValue> ArrayReader for ByteArrayReader<I> {
}
fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
- read_records(&mut self.record_reader, self.pages.as_mut(), batch_size)?;
+ let size = self.read_records(batch_size)?;
Review Comment:
Perhaps we could make this the default impl on the trait?
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +196,15 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = self.data_buffer.split_off(batch_size);
+ if let Some(buf) = &mut self.def_levels_buffer {
Review Comment:
I think if we remove the `batch_size` argument to `consume_batch` we won't need to make this change
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -349,30 +398,32 @@ mod tests {
let mut accu_len: usize = 0;
- let array = array_reader.next_batch(values_per_page / 2).unwrap();
- assert_eq!(array.len(), values_per_page / 2);
+ let len = array_reader.read_records(values_per_page / 2).unwrap();
Review Comment:
Again I think removing batch_size from consume_batch allows preserving the existing behaviour
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933961394
##########
parquet/src/arrow/array_reader/byte_array.rs:
##########
@@ -109,7 +109,23 @@ impl<I: OffsetSizeTrait + ScalarValue> ArrayReader for ByteArrayReader<I> {
}
fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
- read_records(&mut self.record_reader, self.pages.as_mut(), batch_size)?;
+ let size = self.read_records(batch_size)?;
Review Comment:
Yes.
--
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 #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r934558403
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +181,10 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = vec![];
+ self.def_levels_buffer = None;
+ self.rep_levels_buffer = None;
Review Comment:
As I suspected the test added in #2254 is now failing, I think it should be simple enough to fix, but let me know if you get stuck :smile:
--
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 #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237
--
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 #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933991080
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +181,10 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = vec![];
+ self.def_levels_buffer = None;
+ self.rep_levels_buffer = None;
Review Comment:
If StructArray reader is itself nullable it needs to read the definition levels read by its child in order to work out where it's NULLs are located. It's a similar story for ListArrayReader.
I'll try to rustle up some tests this evening, so that we can be confident this PR won't break anything. Longer term I want to remove ComplexObjectArrayReader as it is slow, complicated and largely replaced by the newer generics.
--
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 #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933983857
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +181,10 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = vec![];
+ self.def_levels_buffer = None;
+ self.rep_levels_buffer = None;
Review Comment:
I think this will break, as parent ArrayReader assume the definition levels live until the next call to consume_batch?
I'm not sure we actually have test coverage of say a nullable StructArray containing a DecimalArray π€
--
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] Ted-Jiang commented on pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#issuecomment-1200107817
@tustvold 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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933961877
##########
parquet/src/arrow/array_reader/empty_array.rs:
##########
@@ -54,14 +54,21 @@ impl ArrayReader for EmptyArrayReader {
}
fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
+ let size = self.read_records(batch_size)?;
+ self.consume_batch(size)
+ }
+
+ fn read_records(&mut self, batch_size: usize) -> Result<usize> {
let len = self.remaining_rows.min(batch_size);
self.remaining_rows -= len;
+ Ok(len)
+ }
+ fn consume_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
let data = ArrayDataBuilder::new(self.data_type.clone())
- .len(len)
+ .len(batch_size)
Review Comment:
Yes, will add one field in struct π
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933755843
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -349,30 +398,32 @@ mod tests {
let mut accu_len: usize = 0;
- let array = array_reader.next_batch(values_per_page / 2).unwrap();
- assert_eq!(array.len(), values_per_page / 2);
+ let len = array_reader.read_records(values_per_page / 2).unwrap();
Review Comment:
Now, after `consume_batch` we will clean up the buffer which has been used.
So now we should get `get_def_levels` after `read_records` before `consume_batch`
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933966267
##########
parquet/src/arrow/array_reader/mod.rs:
##########
@@ -115,7 +125,7 @@ impl RowGroupCollection for Arc<dyn FileReader> {
///
/// Returns the number of records read, which can be less than `batch_size` if
/// pages is exhausted.
-fn read_records<V, CV>(
+fn read_records_inner<V, CV>(
Review Comment:
i think there two `read_records `, one in "ArrayReader", another in "array_reader" mod, just to want distinct themπ, I will get it back.
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933967600
##########
parquet/src/arrow/array_reader/byte_array.rs:
##########
@@ -109,7 +109,23 @@ impl<I: OffsetSizeTrait + ScalarValue> ArrayReader for ByteArrayReader<I> {
}
fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
- read_records(&mut self.record_reader, self.pages.as_mut(), batch_size)?;
+ let size = self.read_records(batch_size)?;
+ self.consume_batch(size)
+ }
+
+ fn read_records(&mut self, batch_size: usize) -> Result<usize> {
+ read_records_inner(&mut self.record_reader, self.pages.as_mut(), batch_size)
+ }
+
+ fn consume_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
Review Comment:
@tustvold fix in [fix comment](https://github.com/apache/arrow-rs/pull/2237/commits/bc07bdb738dccacdf26c951458e9e89428a4c8d3)
--
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 pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#issuecomment-1201227780
I took the liberty of merging in master to get #2254
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r936190661
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -206,11 +213,19 @@ where
}
fn get_def_levels(&self) -> Option<&[i16]> {
- self.def_levels_buffer.as_deref()
+ if self.before_consume {
Review Comment:
without this, will fail in
```
failures:
arrow::array_reader::complex_object_array::tests::test_complex_array_reader_def_and_rep_levels
arrow::array_reader::complex_object_array::tests::test_complex_array_reader_dict_enc_string
```
this cause by `get_level` before consume `complex_object_array` (this is the common situation like other readers), but `complex_object_array` sometimes(self is nullable) need `get_level` after consume, so i think we should keep this check
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r935690653
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +181,10 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = vec![];
+ self.def_levels_buffer = None;
+ self.rep_levels_buffer = None;
Review Comment:
@tustvold Sorry for the late reply, fix at [issue_2197](https://github.com/Ted-Jiang/arrow-rs/tree/issue_2197)
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r935693887
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -1161,7 +1161,7 @@ mod tests {
Some(props),
)
.expect("Unable to write file");
- writer.write(&expected_batch).unwrap();
+ writer.write(expected_batch).unwrap();
Review Comment:
Not related, but got clippy error at local.
--
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] Ted-Jiang commented on a diff in pull request #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r933961275
##########
parquet/src/arrow/array_reader/byte_array.rs:
##########
@@ -109,7 +109,23 @@ impl<I: OffsetSizeTrait + ScalarValue> ArrayReader for ByteArrayReader<I> {
}
fn next_batch(&mut self, batch_size: usize) -> Result<ArrayRef> {
- read_records(&mut self.record_reader, self.pages.as_mut(), batch_size)?;
+ let size = self.read_records(batch_size)?;
Review Comment:
Yes.
--
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 #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#discussion_r934558403
##########
parquet/src/arrow/array_reader/complex_object_array.rs:
##########
@@ -160,6 +181,10 @@ where
array = arrow::compute::cast(&array, &self.data_type)?;
}
+ self.data_buffer = vec![];
+ self.def_levels_buffer = None;
+ self.rep_levels_buffer = None;
Review Comment:
As I suspected the test added in #2254 is now failing, I think it should be simple enough to fix though :smile:
--
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 #2237: Separate ArrayReader::next_batch with read_records and consume_batch
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2237:
URL: https://github.com/apache/arrow-rs/pull/2237#issuecomment-1200096879
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/2237?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 [#2237](https://codecov.io/gh/apache/arrow-rs/pull/2237?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (76b7d40) into [master](https://codecov.io/gh/apache/arrow-rs/commit/985760f5609125f1ca4797c1df9ba69ab01aab85?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (985760f) will **decrease** coverage by `0.24%`.
> The diff coverage is `80.13%`.
```diff
@@ Coverage Diff @@
## master #2237 +/- ##
==========================================
- Coverage 82.53% 82.28% -0.25%
==========================================
Files 239 240 +1
Lines 62304 62541 +237
==========================================
+ Hits 51422 51463 +41
- Misses 10882 11078 +196
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/2237?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Ξ | |
|---|---|---|
| [parquet/src/arrow/array\_reader/byte\_array.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL2J5dGVfYXJyYXkucnM=) | `84.81% <40.00%> (-1.52%)` | :arrow_down: |
| [...et/src/arrow/array\_reader/byte\_array\_dictionary.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL2J5dGVfYXJyYXlfZGljdGlvbmFyeS5ycw==) | `85.03% <45.45%> (-1.94%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader/null\_array.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL251bGxfYXJyYXkucnM=) | `74.19% <60.00%> (-7.29%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader/map\_array.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL21hcF9hcnJheS5ycw==) | `57.69% <66.66%> (-1.14%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader/test\_util.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL3Rlc3RfdXRpbC5ycw==) | `75.60% <75.00%> (-2.03%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader/struct\_array.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL3N0cnVjdF9hcnJheS5ycw==) | `85.18% <76.47%> (-1.37%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader/empty\_array.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL2VtcHR5X2FycmF5LnJz) | `55.55% <83.33%> (+10.10%)` | :arrow_up: |
| [...uet/src/arrow/array\_reader/complex\_object\_array.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL2NvbXBsZXhfb2JqZWN0X2FycmF5LnJz) | `93.06% <92.75%> (-0.97%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader/list\_array.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL2xpc3RfYXJyYXkucnM=) | `92.85% <100.00%> (+0.16%)` | :arrow_up: |
| [parquet/src/arrow/array\_reader/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/2237/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-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL21vZC5ycw==) | `92.59% <100.00%> (ΓΈ)` | |
| ... and [25 more](https://codecov.io/gh/apache/arrow-rs/pull/2237/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?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