marvinlanhenke commented on code in PR #347:
URL: https://github.com/apache/iceberg-rust/pull/347#discussion_r1582148013
##########
crates/iceberg/src/scan.rs:
##########
@@ -218,6 +225,25 @@ impl TableScan {
let mut manifest_entries =
iter(manifest.entries().iter().filter(|e| e.is_alive()));
while let Some(manifest_entry) = manifest_entries.next().await
{
+
+ // If this scan has a filter, check the data file
evaluator cache for an existing
+ // InclusiveMetricsEvaluator that matches this manifest's
partition spec ID.
+ // Use one from the cache if there is one. If not, create
one, put it in
+ // the cache, and take a reference to it.
+ if let Some(filter) = filter.as_ref() {
+ #[allow(clippy::map_entry)]
+ if
!data_file_evaluator_cache.contains_key(&entry.partition_spec_id) {
Review Comment:
Do we even need to cache the InclusiveMetricEval?
From my understanding the python
[impl](https://github.com/apache/iceberg-python/blob/4b96d2f49b04ff7ec551646f489ecc50ac195b5d/pyiceberg/table/__init__.py#L1695-L1697)
doesn't do this. So we might just need one instance (outside of try_stream!)
of the MetricsEvaluator and call `eval()` on each `DataFile`?
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -0,0 +1,657 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::expr::visitors::bound_predicate_visitor::{visit,
BoundPredicateVisitor};
+use crate::expr::{BoundPredicate, BoundReference};
+use crate::spec::{DataFile, Datum, Literal, PrimitiveLiteral};
+use crate::{Error, ErrorKind};
+use fnv::FnvHashSet;
+
+const IN_PREDICATE_LIMIT: usize = 200;
+const ROWS_MIGHT_MATCH: crate::Result<bool> = Ok(true);
+const ROWS_CANNOT_MATCH: crate::Result<bool> = Ok(false);
+
+pub(crate) struct InclusiveMetricsEvaluator {
+ include_empty_files: bool,
+ filter: BoundPredicate,
+}
+
+impl InclusiveMetricsEvaluator {
+ pub(crate) fn new(filter: BoundPredicate, include_empty_files: bool) ->
crate::Result<Self> {
+ Ok(InclusiveMetricsEvaluator {
+ include_empty_files,
+ filter,
+ })
+ }
+
+ /// Evaluate this `InclusiveMetricsEvaluator`'s filter predicate against
the
+ /// provided [`DataFile`]'s partitions. Used by [`TableScan`] to
Review Comment:
```suggestion
/// provided [`DataFile`]'s. Used by [`TableScan`] to
```
I think we evaluate the DataFile itself e.g. lower / upper_bounds etc. not
only the partitions?
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -0,0 +1,657 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::expr::visitors::bound_predicate_visitor::{visit,
BoundPredicateVisitor};
+use crate::expr::{BoundPredicate, BoundReference};
+use crate::spec::{DataFile, Datum, Literal, PrimitiveLiteral};
+use crate::{Error, ErrorKind};
+use fnv::FnvHashSet;
+
+const IN_PREDICATE_LIMIT: usize = 200;
+const ROWS_MIGHT_MATCH: crate::Result<bool> = Ok(true);
+const ROWS_CANNOT_MATCH: crate::Result<bool> = Ok(false);
+
+pub(crate) struct InclusiveMetricsEvaluator {
+ include_empty_files: bool,
+ filter: BoundPredicate,
+}
+
+impl InclusiveMetricsEvaluator {
+ pub(crate) fn new(filter: BoundPredicate, include_empty_files: bool) ->
crate::Result<Self> {
+ Ok(InclusiveMetricsEvaluator {
+ include_empty_files,
+ filter,
+ })
+ }
+
+ /// Evaluate this `InclusiveMetricsEvaluator`'s filter predicate against
the
+ /// provided [`DataFile`]'s partitions. Used by [`TableScan`] to
+ /// see if this `DataFile` contains data that matches
+ /// the scan's filter.
+ pub(crate) fn eval(&self, data_file: &DataFile) -> crate::Result<bool> {
+ if !self.include_empty_files && data_file.record_count == 0 {
+ return ROWS_CANNOT_MATCH;
+ }
+
+ let mut evaluator = DataFileFilterVisitor::new(self, data_file);
+
+ visit(&mut evaluator, &self.filter)
+ }
+}
+
+struct DataFileFilterVisitor<'a> {
Review Comment:
just to verify my understanding: You implemented the `BoundPredicateVisitor`
not on `InclusiveMetricsEvaluator` in order to avoid needing an owned
`DataFile`? This way you can pass a ref &DataFile to eval()?
nit: rename to `InclusiveMetricsVisitor` ?
##########
crates/iceberg/src/scan.rs:
##########
@@ -262,6 +288,16 @@ impl TableScan {
)
}
+ fn create_data_file_evaluator(
Review Comment:
nit: rename to `create_inclusive_metrics_evaluator`?
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -0,0 +1,657 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::expr::visitors::bound_predicate_visitor::{visit,
BoundPredicateVisitor};
+use crate::expr::{BoundPredicate, BoundReference};
+use crate::spec::{DataFile, Datum, Literal, PrimitiveLiteral};
+use crate::{Error, ErrorKind};
+use fnv::FnvHashSet;
+
+const IN_PREDICATE_LIMIT: usize = 200;
+const ROWS_MIGHT_MATCH: crate::Result<bool> = Ok(true);
+const ROWS_CANNOT_MATCH: crate::Result<bool> = Ok(false);
+
+pub(crate) struct InclusiveMetricsEvaluator {
+ include_empty_files: bool,
+ filter: BoundPredicate,
+}
+
+impl InclusiveMetricsEvaluator {
+ pub(crate) fn new(filter: BoundPredicate, include_empty_files: bool) ->
crate::Result<Self> {
+ Ok(InclusiveMetricsEvaluator {
+ include_empty_files,
+ filter,
+ })
+ }
+
+ /// Evaluate this `InclusiveMetricsEvaluator`'s filter predicate against
the
+ /// provided [`DataFile`]'s partitions. Used by [`TableScan`] to
+ /// see if this `DataFile` contains data that matches
+ /// the scan's filter.
+ pub(crate) fn eval(&self, data_file: &DataFile) -> crate::Result<bool> {
+ if !self.include_empty_files && data_file.record_count == 0 {
+ return ROWS_CANNOT_MATCH;
+ }
+
+ let mut evaluator = DataFileFilterVisitor::new(self, data_file);
+
+ visit(&mut evaluator, &self.filter)
+ }
+}
+
+struct DataFileFilterVisitor<'a> {
+ inclusive_metrics_evaluator: &'a InclusiveMetricsEvaluator,
+ data_file: &'a DataFile,
+}
+
+// Remove this annotation once all todos have been removed
+#[allow(unused_variables)]
+impl BoundPredicateVisitor for DataFileFilterVisitor<'_> {
+ type T = bool;
+
+ fn always_true(&mut self) -> crate::Result<bool> {
+ ROWS_MIGHT_MATCH
+ }
+
+ fn always_false(&mut self) -> crate::Result<bool> {
+ ROWS_CANNOT_MATCH
+ }
+
+ fn and(&mut self, lhs: bool, rhs: bool) -> crate::Result<bool> {
+ Ok(lhs && rhs)
+ }
+
+ fn or(&mut self, lhs: bool, rhs: bool) -> crate::Result<bool> {
+ Ok(lhs || rhs)
+ }
+
+ fn not(&mut self, inner: bool) -> crate::Result<bool> {
+ Ok(!inner)
+ }
+
+ fn is_null(
+ &mut self,
+ reference: &BoundReference,
+ _predicate: &BoundPredicate,
+ ) -> crate::Result<bool> {
+ let field_id = reference.field().id;
+
+ if let Some(&null_count) = self.null_count(field_id) {
+ if null_count == 0 {
+ ROWS_CANNOT_MATCH
+ } else {
+ ROWS_MIGHT_MATCH
+ }
+ } else {
+ ROWS_MIGHT_MATCH
+ }
+ }
+
+ fn not_null(
+ &mut self,
+ reference: &BoundReference,
+ _predicate: &BoundPredicate,
+ ) -> crate::Result<bool> {
+ let field_id = reference.field().id;
+
+ if self.contains_nulls_only(field_id) {
+ ROWS_CANNOT_MATCH
+ } else {
+ ROWS_MIGHT_MATCH
+ }
Review Comment:
```suggestion
if self.contains_nulls_only(field_id) {
return ROWS_CANNOT_MATCH;
}
ROWS_MIGHT_MATCH
```
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -0,0 +1,657 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::expr::visitors::bound_predicate_visitor::{visit,
BoundPredicateVisitor};
+use crate::expr::{BoundPredicate, BoundReference};
+use crate::spec::{DataFile, Datum, Literal, PrimitiveLiteral};
+use crate::{Error, ErrorKind};
+use fnv::FnvHashSet;
+
+const IN_PREDICATE_LIMIT: usize = 200;
+const ROWS_MIGHT_MATCH: crate::Result<bool> = Ok(true);
+const ROWS_CANNOT_MATCH: crate::Result<bool> = Ok(false);
+
+pub(crate) struct InclusiveMetricsEvaluator {
+ include_empty_files: bool,
+ filter: BoundPredicate,
+}
+
+impl InclusiveMetricsEvaluator {
+ pub(crate) fn new(filter: BoundPredicate, include_empty_files: bool) ->
crate::Result<Self> {
+ Ok(InclusiveMetricsEvaluator {
+ include_empty_files,
+ filter,
+ })
+ }
+
+ /// Evaluate this `InclusiveMetricsEvaluator`'s filter predicate against
the
+ /// provided [`DataFile`]'s partitions. Used by [`TableScan`] to
+ /// see if this `DataFile` contains data that matches
+ /// the scan's filter.
+ pub(crate) fn eval(&self, data_file: &DataFile) -> crate::Result<bool> {
+ if !self.include_empty_files && data_file.record_count == 0 {
+ return ROWS_CANNOT_MATCH;
+ }
+
+ let mut evaluator = DataFileFilterVisitor::new(self, data_file);
+
+ visit(&mut evaluator, &self.filter)
+ }
+}
+
+struct DataFileFilterVisitor<'a> {
+ inclusive_metrics_evaluator: &'a InclusiveMetricsEvaluator,
+ data_file: &'a DataFile,
+}
+
+// Remove this annotation once all todos have been removed
+#[allow(unused_variables)]
+impl BoundPredicateVisitor for DataFileFilterVisitor<'_> {
+ type T = bool;
+
+ fn always_true(&mut self) -> crate::Result<bool> {
+ ROWS_MIGHT_MATCH
+ }
+
+ fn always_false(&mut self) -> crate::Result<bool> {
+ ROWS_CANNOT_MATCH
+ }
+
+ fn and(&mut self, lhs: bool, rhs: bool) -> crate::Result<bool> {
+ Ok(lhs && rhs)
+ }
+
+ fn or(&mut self, lhs: bool, rhs: bool) -> crate::Result<bool> {
+ Ok(lhs || rhs)
+ }
+
+ fn not(&mut self, inner: bool) -> crate::Result<bool> {
+ Ok(!inner)
+ }
+
+ fn is_null(
+ &mut self,
+ reference: &BoundReference,
+ _predicate: &BoundPredicate,
+ ) -> crate::Result<bool> {
+ let field_id = reference.field().id;
+
+ if let Some(&null_count) = self.null_count(field_id) {
+ if null_count == 0 {
+ ROWS_CANNOT_MATCH
+ } else {
+ ROWS_MIGHT_MATCH
+ }
+ } else {
+ ROWS_MIGHT_MATCH
+ }
+ }
+
+ fn not_null(
+ &mut self,
+ reference: &BoundReference,
+ _predicate: &BoundPredicate,
+ ) -> crate::Result<bool> {
+ let field_id = reference.field().id;
+
+ if self.contains_nulls_only(field_id) {
+ ROWS_CANNOT_MATCH
+ } else {
+ ROWS_MIGHT_MATCH
+ }
Review Comment:
also a nit: but sometimes you use an "early-return" sometimes you don't;
just to keep it consistent - and personally I think using "early returns" is
easier to read.
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -0,0 +1,657 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::expr::visitors::bound_predicate_visitor::{visit,
BoundPredicateVisitor};
+use crate::expr::{BoundPredicate, BoundReference};
+use crate::spec::{DataFile, Datum, Literal, PrimitiveLiteral};
+use crate::{Error, ErrorKind};
+use fnv::FnvHashSet;
+
+const IN_PREDICATE_LIMIT: usize = 200;
+const ROWS_MIGHT_MATCH: crate::Result<bool> = Ok(true);
+const ROWS_CANNOT_MATCH: crate::Result<bool> = Ok(false);
+
+pub(crate) struct InclusiveMetricsEvaluator {
+ include_empty_files: bool,
+ filter: BoundPredicate,
+}
+
+impl InclusiveMetricsEvaluator {
+ pub(crate) fn new(filter: BoundPredicate, include_empty_files: bool) ->
crate::Result<Self> {
+ Ok(InclusiveMetricsEvaluator {
+ include_empty_files,
+ filter,
+ })
+ }
+
+ /// Evaluate this `InclusiveMetricsEvaluator`'s filter predicate against
the
+ /// provided [`DataFile`]'s partitions. Used by [`TableScan`] to
+ /// see if this `DataFile` contains data that matches
+ /// the scan's filter.
+ pub(crate) fn eval(&self, data_file: &DataFile) -> crate::Result<bool> {
+ if !self.include_empty_files && data_file.record_count == 0 {
+ return ROWS_CANNOT_MATCH;
+ }
+
+ let mut evaluator = DataFileFilterVisitor::new(self, data_file);
+
+ visit(&mut evaluator, &self.filter)
+ }
+}
+
+struct DataFileFilterVisitor<'a> {
+ inclusive_metrics_evaluator: &'a InclusiveMetricsEvaluator,
+ data_file: &'a DataFile,
+}
+
+// Remove this annotation once all todos have been removed
+#[allow(unused_variables)]
+impl BoundPredicateVisitor for DataFileFilterVisitor<'_> {
+ type T = bool;
+
+ fn always_true(&mut self) -> crate::Result<bool> {
+ ROWS_MIGHT_MATCH
+ }
+
+ fn always_false(&mut self) -> crate::Result<bool> {
+ ROWS_CANNOT_MATCH
+ }
+
+ fn and(&mut self, lhs: bool, rhs: bool) -> crate::Result<bool> {
+ Ok(lhs && rhs)
+ }
+
+ fn or(&mut self, lhs: bool, rhs: bool) -> crate::Result<bool> {
+ Ok(lhs || rhs)
+ }
+
+ fn not(&mut self, inner: bool) -> crate::Result<bool> {
+ Ok(!inner)
+ }
+
+ fn is_null(
+ &mut self,
+ reference: &BoundReference,
+ _predicate: &BoundPredicate,
+ ) -> crate::Result<bool> {
+ let field_id = reference.field().id;
+
+ if let Some(&null_count) = self.null_count(field_id) {
+ if null_count == 0 {
+ ROWS_CANNOT_MATCH
+ } else {
+ ROWS_MIGHT_MATCH
+ }
+ } else {
+ ROWS_MIGHT_MATCH
+ }
Review Comment:
nit: but perhaps more readable to use match here?
```suggestion
match self.null_count(field_id) {
Some(&null_count) if null_count == 0 => ROWS_CANNOT_MATCH,
Some(_) => ROWS_MIGHT_MATCH,
None => ROWS_MIGHT_MATCH,
}
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
