You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/12/20 16:05:19 UTC
[PR] Minor: Improve comments and errors for ArrowPredicate [arrow-rs]
alamb opened a new pull request, #5230:
URL: https://github.com/apache/arrow-rs/pull/5230
# Which issue does this PR close?
N/A
# Rationale for this change
While debugging https://github.com/apache/arrow-datafusion/issues/8600 it took me a while to figure out what was wrong and I think some better comments and error messages would have helped.
# What changes are included in this PR?
Encode some invariants in the comments and error messages.
# Are there any user-facing changes?
Better docs and error message
<!--
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
Re: [PR] Minor: Improve comments and errors for ArrowPredicate [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5230:
URL: https://github.com/apache/arrow-rs/pull/5230#discussion_r1433080912
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -677,11 +677,13 @@ pub(crate) fn apply_range(
selection
}
-/// Evaluates an [`ArrowPredicate`] returning the [`RowSelection`]
+/// Evaluates an [`ArrowPredicate`], returning a [`RowSelection`] indiciating
+/// which rows to return.
///
-/// If this [`ParquetRecordBatchReader`] has a [`RowSelection`], the
-/// returned [`RowSelection`] will be the conjunction of this and
-/// the rows selected by `predicate`
+/// If the [`ParquetRecordBatchReader`] also specified an explicit
+/// [`RowSelection`] in addition to a predicate, `input_selection` will be
Review Comment:
IIRC you will also get an input_selection if you have multiple predicates, as it will chain them
--
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
Re: [PR] Minor: Improve comments and errors for ArrowPredicate [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5230:
URL: https://github.com/apache/arrow-rs/pull/5230#discussion_r1432908085
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -691,7 +693,16 @@ pub(crate) fn evaluate_predicate(
let reader = ParquetRecordBatchReader::new(batch_size, array_reader, input_selection.clone());
let mut filters = vec![];
for maybe_batch in reader {
- let filter = predicate.evaluate(maybe_batch?)?;
+ let maybe_batch = maybe_batch?;
+ let input_rows = maybe_batch.num_rows();
+ let filter = predicate.evaluate(maybe_batch)?;
+ // Since user supplied predicate, check error here to catch bugs quickly
+ if filter.len() != input_rows {
Review Comment:
If this doesn't hold true, `RowSelection::and_then` panics later. Getting a more specific error closer to where the bug happened would have helped me a lot
--
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
Re: [PR] Minor: Improve comments and errors for ArrowPredicate [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #5230:
URL: https://github.com/apache/arrow-rs/pull/5230
--
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
Re: [PR] Minor: Improve comments and errors for ArrowPredicate [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5230:
URL: https://github.com/apache/arrow-rs/pull/5230#discussion_r1433159121
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -677,11 +677,13 @@ pub(crate) fn apply_range(
selection
}
-/// Evaluates an [`ArrowPredicate`] returning the [`RowSelection`]
+/// Evaluates an [`ArrowPredicate`], returning a [`RowSelection`] indiciating
+/// which rows to return.
///
-/// If this [`ParquetRecordBatchReader`] has a [`RowSelection`], the
-/// returned [`RowSelection`] will be the conjunction of this and
-/// the rows selected by `predicate`
+/// If the [`ParquetRecordBatchReader`] also specified an explicit
+/// [`RowSelection`] in addition to a predicate, `input_selection` will be
Review Comment:
I double checked and you are correct. I updated the comments to reflect this detai
--
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