You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "sdd (via GitHub)" <gi...@apache.org> on 2024/03/09 14:09:53 UTC
[PR] [WIP] Add `PartitionEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
sdd opened a new pull request, #241:
URL: https://github.com/apache/iceberg-rust/pull/241
A very early-stage PR to propose an initial design for a partition evaluator to reject files in the table scan's file planner that don't match the scan's filter.
Which issue does this PR close?
Closes https://github.com/apache/iceberg-rust/issues/152 partly.
Rationale for this change
Add the ability to use the TableScan's filter in the file planner to filter out files whose partition values don't match the filter.
What changes are included in this PR?
* Add an optional `Predicate` to `TableScan`, with which to filter the files (and eventually rows) returned by the scan
* Add a `case_sensitive` flag to `TableScan` to (eventually) allow case-sensitivity of filtering to be selectable
* Add `PartitionEvaluator`: a new struct that handles evaluating whether data files (and eventually manifests, perhaps rows?) should be present in the results of a scan
* Creates a cache of `PartitionEvaluator`s, using its entries to filter out manifest entries that don't pass the scan's filter.
Are these changes tested?
Not yet. Unit and integration tests to follow with the implementation, once the design has been approved.
Are there any user-facing changes?
No.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2032233828
> which is somehow motivated by @viirya 's [pr](https://github.com/apache/iceberg-rust/pull/295/files#diff-a59622727cd67153abdf02031475bf8a1b1921738df4ca9903a685ff6970b7aaR472), but move the travsering flow out of trait body.
@liurenjie1024
...so the traversal flow could then be implemented on e.g. the `ManifestEvaluator` itself.
For example, `eval()` could call the corresponding 'visits` on the visitor that implements `BoundPredicateVisitor`. Is this what you mean?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1550844497
##########
crates/iceberg/src/expr/visitors/bound_predicate_evaluator.rs:
##########
@@ -0,0 +1,339 @@
+// 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::{BoundPredicate, BoundReference, PredicateOperator};
+use crate::spec::Datum;
+use crate::Result;
+use fnv::FnvHashSet;
+
+pub trait BoundPredicateEvaluator {
+ fn visit(&mut self, node: &BoundPredicate) -> Result<bool> {
+ match node {
Review Comment:
according to the design suggestion by @liurenjie1024 [link](https://github.com/apache/iceberg-rust/pull/241#issuecomment-2033371130) this function should live outside the trait as `fn visit_bound_predicate`. Other than that traversal logic looks fine.
##########
crates/iceberg/src/expr/visitors/bound_predicate_evaluator.rs:
##########
@@ -0,0 +1,339 @@
+// 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::{BoundPredicate, BoundReference, PredicateOperator};
+use crate::spec::Datum;
+use crate::Result;
+use fnv::FnvHashSet;
+
+pub trait BoundPredicateEvaluator {
Review Comment:
nit: Perhaps we can move the complete trait into /visitors/mod.rs? However, I am not sure about this - so I'd appreciate your thoughts on this.
##########
crates/iceberg/src/expr/visitors/manifest_evaluator.rs:
##########
@@ -0,0 +1,181 @@
+// 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_evaluator::BoundPredicateEvaluator;
+use crate::expr::visitors::inclusive_projection::InclusiveProjection;
+use crate::expr::{Bind, BoundPredicate, BoundReference};
+use crate::spec::{Datum, FieldSummary, ManifestFile, PartitionSpecRef, Schema, SchemaRef};
+use fnv::FnvHashSet;
+use std::sync::Arc;
+
+pub(crate) struct ManifestEvaluatorFactory {
+ partition_schema: SchemaRef,
+ partition_filter: BoundPredicate,
+ case_sensitive: bool,
+}
+
+impl ManifestEvaluatorFactory {
Review Comment:
Perhaps we can turn this into a proper `Builder` similar to the [python function](https://github.com/apache/iceberg-python/blob/main/pyiceberg/table/__init__.py#L1485). I'm thinking `ManifestEvaluatorBuilder` as a derived `TypedBuilder` would probably be enough. This way we can provide `table_schema`, `partition_spec` and the `row_filter` and orchestrate the `InclusiveProjection` when we call `eval` on the `ManifestEvaluator` itself? What do you think?
##########
crates/iceberg/src/expr/visitors/bound_predicate_evaluator.rs:
##########
@@ -0,0 +1,339 @@
+// 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::{BoundPredicate, BoundReference, PredicateOperator};
+use crate::spec::Datum;
+use crate::Result;
+use fnv::FnvHashSet;
+
+pub trait BoundPredicateEvaluator {
Review Comment:
I think, it'd be better to rename this to `BoundPredicateVisitor`
##########
crates/iceberg/src/expr/visitors/inclusive_projection.rs:
##########
@@ -0,0 +1,86 @@
+// 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::{BoundPredicate, Predicate};
+use crate::spec::{PartitionField, PartitionSpecRef, SchemaRef};
+
+pub(crate) struct InclusiveProjection {
+ #[allow(dead_code)]
+ table_schema: SchemaRef,
+ partition_spec: PartitionSpecRef,
+}
+
+impl InclusiveProjection {
Review Comment:
I'll guess this will be done in the new PR - `InclusiveProjection` should impl `pub trait BoundPredicateVisitor`
##########
crates/iceberg/src/expr/visitors/manifest_evaluator.rs:
##########
@@ -0,0 +1,181 @@
+// 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_evaluator::BoundPredicateEvaluator;
+use crate::expr::visitors::inclusive_projection::InclusiveProjection;
+use crate::expr::{Bind, BoundPredicate, BoundReference};
+use crate::spec::{Datum, FieldSummary, ManifestFile, PartitionSpecRef, Schema, SchemaRef};
+use fnv::FnvHashSet;
+use std::sync::Arc;
+
+pub(crate) struct ManifestEvaluatorFactory {
+ partition_schema: SchemaRef,
+ partition_filter: BoundPredicate,
+ case_sensitive: bool,
+}
+
+impl ManifestEvaluatorFactory {
+ pub(crate) fn new(
+ partition_spec: PartitionSpecRef,
+ table_schema: SchemaRef,
+ partition_filter: BoundPredicate,
+ case_sensitive: bool,
+ ) -> crate::Result<Self> {
+ let partition_type = partition_spec.partition_type(&table_schema)?;
+
+ // this is needed as SchemaBuilder.with_fields expects an iterator over
+ // Arc<NestedField> rather than &Arc<NestedField>
+ let cloned_partition_fields: Vec<_> =
+ partition_type.fields().iter().map(Arc::clone).collect();
+
+ let partition_schema = Schema::builder()
+ .with_fields(cloned_partition_fields)
+ .build()?;
+
+ let partition_schema_ref = Arc::new(partition_schema);
+
+ let inclusive_projection =
+ InclusiveProjection::new(table_schema.clone(), partition_spec.clone());
+ let unbound_partition_filter = inclusive_projection.project(&partition_filter)?;
+
+ let partition_filter =
+ unbound_partition_filter.bind(partition_schema_ref.clone(), case_sensitive)?;
+
+ Ok(Self {
+ partition_schema: partition_schema_ref,
+ partition_filter,
+ case_sensitive,
+ })
+ }
+
+ pub(crate) fn evaluate(&self, manifest_file: &ManifestFile) -> crate::Result<bool> {
Review Comment:
nit: rename to `eval`
##########
crates/iceberg/src/expr/visitors/inclusive_projection.rs:
##########
@@ -0,0 +1,86 @@
+// 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::{BoundPredicate, Predicate};
+use crate::spec::{PartitionField, PartitionSpecRef, SchemaRef};
+
+pub(crate) struct InclusiveProjection {
+ #[allow(dead_code)]
+ table_schema: SchemaRef,
+ partition_spec: PartitionSpecRef,
+}
+
+impl InclusiveProjection {
+ pub(crate) fn new(table_schema: SchemaRef, partition_spec: PartitionSpecRef) -> Self {
+ Self {
+ table_schema,
+ partition_spec,
+ }
+ }
+
+ pub(crate) fn project(&self, predicate: &BoundPredicate) -> crate::Result<Predicate> {
Review Comment:
nit: although it does align with the Python impl. I'd rename this to `eval` to keep it consistent across all `visitors`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `PartitionEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-1986868266
@liurenjie1024 PTAL: Your thoughts on this very much appreciated. This also depends on your predicate binding PR, https://github.com/apache/iceberg-rust/pull/231, getting merged.
@Fokko @Xuanwo @ZENOTME @JanKaul would also appreciate any of your thoughts also.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `PartitionEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1527514205
##########
crates/iceberg/src/expr/predicate.rs:
##########
@@ -599,6 +635,14 @@ impl Display for BoundPredicate {
}
}
+pub(crate) trait PredicateVisitor<T> {
Review Comment:
I don't think these are needed anymore.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2031227629
Also I've got some uncommitted changes around having a boolean evaluator trait, similar to your suggestion. It has default implementations for and / or / not / always true / always false, but requires the others to be implemented by any struct implementing the trait.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1533326719
##########
crates/iceberg/src/scan.rs:
##########
@@ -159,6 +190,21 @@ impl TableScan {
while let Some(entry) = entries.next().await {
let manifest = entry.load_manifest(&file_io).await?;
+ // If this scan has a filter, check the partition evaluator cache for an existing
+ // PartitionEvaluator 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) = self.filter.as_ref() {
+ let partition_evaluator = partition_evaluator_cache
+ .entry(manifest.partition_spec_id())
+ .or_insert_with_key(|key| self.create_partition_evaluator(key, filter));
+
+ // reject any manifest files whose partition values don't match the filter.
+ if !partition_evaluator.filter_manifest_file(&entry) {
Review Comment:
Good point - fixed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `PartitionEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-1987506207
Hi, @sdd Thanks for this pr. The filtering process in fact consists of two steps:
1. Filter manifest files in manifest list. This step is relative easy to do, and has no dependency, see [python implementation](https://github.com/apache/iceberg-python/blob/ea9da8856a686eaeda0d5c2be78d5e3102b67c44/pyiceberg/table/__init__.py#L827).
2. Implement partition pruning. This step is relative complicated, since it has several dependencies, such as inclusive projections, data file evaluator, you can see [python implementation](https://github.com/apache/iceberg-python/blob/ea9da8856a686eaeda0d5c2be78d5e3102b67c44/pyiceberg/table/__init__.py#L787).
My suggestion is to finish [ManifestEvalVisitor](https://github.com/apache/iceberg-python/blob/4ed5a144ecad69878cf47292557b17df31ffe5a8/pyiceberg/expressions/visitors.py#L548) first since it's relative easy and has no dependency.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd closed pull request #241: [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152)
URL: https://github.com/apache/iceberg-rust/pull/241
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2031169057
@liurenjie1024 @ZENOTME @sdd @Xuanwo
I'd really appreciate your thoughts on this:
I took a closer look at the work @sdd has already done - and I think in order to proceed it would make sense to split the `ManifestEvaluator`, `InclusiveProjection` and `PartitionEvaluator` into separate modules.
I am thinking about putting each visitor into its own file within `/iceberg/src/expr` (perhaps even another subfolder /visitors) - for example: `/iceberg/src/expr/manifest_evaluator.rs.`
This way, we could create individual issues on each implementation - and work better in parallel.
Perhaps, it also makes sense to provide a trait to make sure each visitor adheres to the same interface
(although I'm not sure this is neccessary right now...)
```Rust
// Pseudo-Example
pub trait BooleanVisitor:
fn eval() -> boolean
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `PartitionEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1519947613
##########
crates/iceberg/src/scan.rs:
##########
@@ -52,6 +62,18 @@ impl<'a> TableScanBuilder<'a> {
self
}
+ /// Sets the scan as being case-insensitive
+ pub fn with_case_insensitivity(mut self) -> Self {
Review Comment:
This API is a bit strange to me. How about using `with_case_sensitive(v: bool)` for clear?
##########
crates/iceberg/src/scan.rs:
##########
@@ -207,6 +268,34 @@ impl FileScanTask {
}
}
+/// Evaluates manifest files to see if their partition values comply with a filter predicate
+pub struct PartitionEvaluator {
+ bound_predicate: BoundPredicate,
+ partition_spec: PartitionSpecRef,
+}
+
+impl PartitionEvaluator {
+ pub(crate) fn new(partition_spec: PartitionSpecRef, bound_predicate: BoundPredicate) -> Self {
+ PartitionEvaluator {
+ bound_predicate,
+ partition_spec,
+ }
+ }
+
+ pub(crate) fn filter_manifest(&self, _manifest: &Manifest) -> bool {
+ // TODO: filter manifests using partition summaries
Review Comment:
How about leaving a `todo!()` here to avoid we forgot this TODO and leak this behavior to end users?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1528671767
##########
crates/iceberg/src/scan.rs:
##########
@@ -159,6 +190,21 @@ impl TableScan {
while let Some(entry) = entries.next().await {
let manifest = entry.load_manifest(&file_io).await?;
+ // If this scan has a filter, check the partition evaluator cache for an existing
+ // PartitionEvaluator 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) = self.filter.as_ref() {
+ let partition_evaluator = partition_evaluator_cache
+ .entry(manifest.partition_spec_id())
+ .or_insert_with_key(|key| self.create_partition_evaluator(key, filter));
+
+ // reject any manifest files whose partition values don't match the filter.
+ if !partition_evaluator.filter_manifest_file(&entry) {
Review Comment:
If I understood correctly; the `ManifestEval` is also applied here? I think this should be done before loading each manifest file by evaluating the `partitions: Vec<FieldSummary>` of `ManifestList`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2033371130
> > which is somehow motivated by @viirya 's [pr](https://github.com/apache/iceberg-rust/pull/295/files#diff-a59622727cd67153abdf02031475bf8a1b1921738df4ca9903a685ff6970b7aaR472), but move the travsering flow out of trait body.
>
> @liurenjie1024 ...so the traversal flow could then be implemented on e.g. the `ManifestEvaluator` itself. For example, `eval()` could call the corresponding 'visit_xx' on the visitor that implements `BoundPredicateVisitor`. Is this what you mean?
I was thinking about following following structure:
```rust
pub trait BoundPredicateVisitor {
type T;
fn visit_and(&mut self, values: [Self::T; 2]) -> Result<Self::T>;
fn visit_or(&mut self, values: [Self::T; 2]) -> Result<Self::T>;
...
}
pub fn visit_bound_predicate(visitor: &mut V, predicate: &BoundPredicate) -> Result<V::T> {
match predicate {
BoundPredicate::And(children) => {
let ret = [visit_bound_predicate(visitor, children[0]),visit_bound_predicate(visitor, children[1])];
visitor.visit_and(ret)
},
...
}
}
pub struct ManifestEvaluator {}
impl BoundPredicateVisitor for ManifestEvaluator {}
impl ManifestEvaluator {
pub fn eval(&mut self, predicate: &BoundPredicate) -> bool {
visit_bound_predicate(self, predicate)?
}
}
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2031222529
Thanks for the review @marvinlanhenke ! I'll address the comments today or tomorrow morning 😁
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1547190841
##########
crates/iceberg/src/scan.rs:
##########
@@ -155,8 +189,22 @@ impl TableScan {
.await?;
// Generate data file stream
- let mut entries = iter(manifest_list.entries());
- while let Some(entry) = entries.next().await {
+ for entry in manifest_list.entries() {
+ // If this scan has a filter, check the partition evaluator cache for an existing
+ // PartitionEvaluator 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) = self.filter.as_ref() {
+ let partition_evaluator = partition_evaluator_cache
+ .entry(entry.partition_spec_id())
+ .or_insert_with_key(|key| self.create_partition_evaluator(key, filter));
+
+ // reject any manifest files whose partition values don't match the filter.
+ if !partition_evaluator.filter_manifest_file(entry) {
Review Comment:
I think we should apply `ManifestEvaluator` independently from `PartitionEvaluator` (see comment down below in struct PartitionEvaluator).
Also, I'm not sure if we can simply 'skip' the file with continue. In the python impl the Partition- and MetricsEvaluator are applied on each DataFile on multiple threads - so I believe we have to collect all relevant manifest files before and then apply the Partition- and MetricsEvaluator on the complete (pre-filtered) collection, in order to split the work into multiple threads?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2035371003
Hi @liurenjie1024 and @marvinlanhenke. I've updated this PR to align with the direction suggested above - thanks a lot for all the feedback!
This PR is clearly far too big to easily digest now though. I'll rebase it and resubmit it in pieces as separate PRs:
* Accessors
* BoundPredicateVisitor
* InclusiveProjection
* ManifestEvaluator itself
* Misc changes such as adding AlwaysTrue / AlwaysFalse and any new getters
* Changes to `scan.rs` that bring it all together
Unless you have any other suggestions. I'll happily close any of the raised PRs if they don't match the direction we agree to go with 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1547117184
##########
crates/iceberg/src/scan.rs:
##########
@@ -214,326 +272,559 @@ impl FileScanTask {
}
}
-#[cfg(test)]
-mod tests {
- use crate::io::{FileIO, OutputFile};
- use crate::spec::{
- DataContentType, DataFileBuilder, DataFileFormat, FormatVersion, Literal, Manifest,
- ManifestContentType, ManifestEntry, ManifestListWriter, ManifestMetadata, ManifestStatus,
- ManifestWriter, Struct, TableMetadata, EMPTY_SNAPSHOT_ID,
- };
- use crate::table::Table;
- use crate::TableIdent;
- use arrow_array::{ArrayRef, Int64Array, RecordBatch};
- use futures::TryStreamExt;
- use parquet::arrow::{ArrowWriter, PARQUET_FIELD_ID_META_KEY};
- use parquet::basic::Compression;
- use parquet::file::properties::WriterProperties;
- use std::collections::HashMap;
- use std::fs;
- use std::fs::File;
- use std::sync::Arc;
- use tempfile::TempDir;
- use tera::{Context, Tera};
- use uuid::Uuid;
-
- struct TableTestFixture {
- table_location: String,
- table: Table,
- }
-
- impl TableTestFixture {
- fn new() -> Self {
- let tmp_dir = TempDir::new().unwrap();
- let table_location = tmp_dir.path().join("table1");
- let manifest_list1_location = table_location.join("metadata/manifests_list_1.avro");
- let manifest_list2_location = table_location.join("metadata/manifests_list_2.avro");
- let table_metadata1_location = table_location.join("metadata/v1.json");
-
- let file_io = FileIO::from_path(table_location.as_os_str().to_str().unwrap())
- .unwrap()
- .build()
- .unwrap();
-
- let table_metadata = {
- let template_json_str = fs::read_to_string(format!(
- "{}/testdata/example_table_metadata_v2.json",
- env!("CARGO_MANIFEST_DIR")
- ))
- .unwrap();
- let mut context = Context::new();
- context.insert("table_location", &table_location);
- context.insert("manifest_list_1_location", &manifest_list1_location);
- context.insert("manifest_list_2_location", &manifest_list2_location);
- context.insert("table_metadata_1_location", &table_metadata1_location);
-
- let metadata_json = Tera::one_off(&template_json_str, &context, false).unwrap();
- serde_json::from_str::<TableMetadata>(&metadata_json).unwrap()
- };
-
- let table = Table::builder()
- .metadata(table_metadata)
- .identifier(TableIdent::from_strs(["db", "table1"]).unwrap())
- .file_io(file_io)
- .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap())
- .build();
-
- Self {
- table_location: table_location.to_str().unwrap().to_string(),
- table,
- }
- }
-
- fn next_manifest_file(&self) -> OutputFile {
- self.table
- .file_io()
- .new_output(format!(
- "{}/metadata/manifest_{}.avro",
- self.table_location,
- Uuid::new_v4()
- ))
- .unwrap()
- }
-
- async fn setup_manifest_files(&mut self) {
- let current_snapshot = self.table.metadata().current_snapshot().unwrap();
- let parent_snapshot = current_snapshot
- .parent_snapshot(self.table.metadata())
- .unwrap();
- let current_schema = current_snapshot.schema(self.table.metadata()).unwrap();
- let current_partition_spec = self.table.metadata().default_partition_spec().unwrap();
-
- // Write data files
- let data_file_manifest = ManifestWriter::new(
- self.next_manifest_file(),
- current_snapshot.snapshot_id(),
- vec![],
- )
- .write(Manifest::new(
- ManifestMetadata::builder()
- .schema((*current_schema).clone())
- .content(ManifestContentType::Data)
- .format_version(FormatVersion::V2)
- .partition_spec((**current_partition_spec).clone())
- .schema_id(current_schema.schema_id())
- .build(),
- vec![
- ManifestEntry::builder()
- .status(ManifestStatus::Added)
- .data_file(
- DataFileBuilder::default()
- .content(DataContentType::Data)
- .file_path(format!("{}/1.parquet", &self.table_location))
- .file_format(DataFileFormat::Parquet)
- .file_size_in_bytes(100)
- .record_count(1)
- .partition(Struct::from_iter([Some(Literal::long(100))]))
- .build()
- .unwrap(),
- )
- .build(),
- ManifestEntry::builder()
- .status(ManifestStatus::Deleted)
- .snapshot_id(parent_snapshot.snapshot_id())
- .sequence_number(parent_snapshot.sequence_number())
- .file_sequence_number(parent_snapshot.sequence_number())
- .data_file(
- DataFileBuilder::default()
- .content(DataContentType::Data)
- .file_path(format!("{}/2.parquet", &self.table_location))
- .file_format(DataFileFormat::Parquet)
- .file_size_in_bytes(100)
- .record_count(1)
- .partition(Struct::from_iter([Some(Literal::long(200))]))
- .build()
- .unwrap(),
- )
- .build(),
- ManifestEntry::builder()
- .status(ManifestStatus::Existing)
- .snapshot_id(parent_snapshot.snapshot_id())
- .sequence_number(parent_snapshot.sequence_number())
- .file_sequence_number(parent_snapshot.sequence_number())
- .data_file(
- DataFileBuilder::default()
- .content(DataContentType::Data)
- .file_path(format!("{}/3.parquet", &self.table_location))
- .file_format(DataFileFormat::Parquet)
- .file_size_in_bytes(100)
- .record_count(1)
- .partition(Struct::from_iter([Some(Literal::long(300))]))
- .build()
- .unwrap(),
- )
- .build(),
- ],
- ))
- .await
- .unwrap();
+/// Evaluates manifest files to see if their partition values comply with a filter predicate
+pub struct PartitionEvaluator {
+ manifest_eval_visitor: ManifestEvalVisitor,
+}
- // Write to manifest list
- let mut manifest_list_write = ManifestListWriter::v2(
- self.table
- .file_io()
- .new_output(current_snapshot.manifest_list())
- .unwrap(),
- current_snapshot.snapshot_id(),
- current_snapshot
- .parent_snapshot_id()
- .unwrap_or(EMPTY_SNAPSHOT_ID),
- current_snapshot.sequence_number(),
- );
- manifest_list_write
- .add_manifests(vec![data_file_manifest].into_iter())
- .unwrap();
- manifest_list_write.close().await.unwrap();
-
- // prepare data
- let schema = {
- let fields =
- vec![
- arrow_schema::Field::new("col", arrow_schema::DataType::Int64, true)
- .with_metadata(HashMap::from([(
- PARQUET_FIELD_ID_META_KEY.to_string(),
- "0".to_string(),
- )])),
- ];
- Arc::new(arrow_schema::Schema::new(fields))
- };
- let col = Arc::new(Int64Array::from_iter_values(vec![1; 1024])) as ArrayRef;
- let to_write = RecordBatch::try_new(schema.clone(), vec![col]).unwrap();
-
- // Write the Parquet files
- let props = WriterProperties::builder()
- .set_compression(Compression::SNAPPY)
- .build();
-
- for n in 1..=3 {
- let file = File::create(format!("{}/{}.parquet", &self.table_location, n)).unwrap();
- let mut writer =
- ArrowWriter::try_new(file, to_write.schema(), Some(props.clone())).unwrap();
-
- writer.write(&to_write).expect("Writing batch");
-
- // writer must be closed to write footer
- writer.close().unwrap();
- }
- }
+impl PartitionEvaluator {
+ pub(crate) fn new(
+ partition_spec: PartitionSpecRef,
+ partition_filter: BoundPredicate,
+ table_schema: SchemaRef,
+ ) -> crate::Result<Self> {
+ let manifest_eval_visitor = ManifestEvalVisitor::manifest_evaluator(
+ partition_spec,
+ table_schema,
+ partition_filter,
+ true,
+ )?;
+
+ Ok(PartitionEvaluator {
+ manifest_eval_visitor,
+ })
}
- #[test]
- fn test_table_scan_columns() {
- let table = TableTestFixture::new().table;
-
- let table_scan = table.scan().select(["x", "y"]).build().unwrap();
- assert_eq!(vec!["x", "y"], table_scan.column_names);
-
- let table_scan = table
- .scan()
- .select(["x", "y"])
- .select(["z"])
- .build()
- .unwrap();
- assert_eq!(vec!["z"], table_scan.column_names);
+ pub(crate) fn filter_manifest_file(&self, _manifest_file: &ManifestFile) -> bool {
+ self.manifest_eval_visitor.eval(_manifest_file)
}
+}
- #[test]
- fn test_select_all() {
- let table = TableTestFixture::new().table;
+struct ManifestEvalVisitor {
+ #[allow(dead_code)]
+ partition_schema: SchemaRef,
+ partition_filter: BoundPredicate,
+ #[allow(dead_code)]
+ case_sensitive: bool,
+}
- let table_scan = table.scan().select_all().build().unwrap();
- assert!(table_scan.column_names.is_empty());
+impl ManifestEvalVisitor {
+ fn new(
+ partition_schema: SchemaRef,
+ partition_filter: Predicate,
+ case_sensitive: bool,
+ ) -> crate::Result<Self> {
+ let partition_filter = partition_filter.bind(partition_schema.clone(), case_sensitive)?;
Review Comment:
I think we have `rewrite_not` already in place to be applied to partition_filter?
##########
crates/iceberg/src/scan.rs:
##########
@@ -155,8 +189,22 @@ impl TableScan {
.await?;
// Generate data file stream
- let mut entries = iter(manifest_list.entries());
- while let Some(entry) = entries.next().await {
+ for entry in manifest_list.entries() {
+ // If this scan has a filter, check the partition evaluator cache for an existing
+ // PartitionEvaluator 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) = self.filter.as_ref() {
+ let partition_evaluator = partition_evaluator_cache
+ .entry(entry.partition_spec_id())
+ .or_insert_with_key(|key| self.create_partition_evaluator(key, filter));
+
+ // reject any manifest files whose partition values don't match the filter.
+ if !partition_evaluator.filter_manifest_file(entry) {
Review Comment:
I think we should apply `ManifestEvaluator` independently from `PartitionEvaluator` (see comment down below in struct PartitionEvaluator).
Also, I'm not sure if we can simply 'skip' the file with continue. In the python impl the Partition- and MetricsEvaluator are applied on each DataFile on multiple threads - so I believe we have to collect all relevant manifest files before and then apply the Partition- and MetricsEvaluator on the complete collections (or split the work into multiple threads)?
##########
crates/iceberg/src/scan.rs:
##########
@@ -214,326 +272,559 @@ impl FileScanTask {
}
}
-#[cfg(test)]
-mod tests {
- use crate::io::{FileIO, OutputFile};
- use crate::spec::{
- DataContentType, DataFileBuilder, DataFileFormat, FormatVersion, Literal, Manifest,
- ManifestContentType, ManifestEntry, ManifestListWriter, ManifestMetadata, ManifestStatus,
- ManifestWriter, Struct, TableMetadata, EMPTY_SNAPSHOT_ID,
- };
- use crate::table::Table;
- use crate::TableIdent;
- use arrow_array::{ArrayRef, Int64Array, RecordBatch};
- use futures::TryStreamExt;
- use parquet::arrow::{ArrowWriter, PARQUET_FIELD_ID_META_KEY};
- use parquet::basic::Compression;
- use parquet::file::properties::WriterProperties;
- use std::collections::HashMap;
- use std::fs;
- use std::fs::File;
- use std::sync::Arc;
- use tempfile::TempDir;
- use tera::{Context, Tera};
- use uuid::Uuid;
-
- struct TableTestFixture {
- table_location: String,
- table: Table,
- }
-
- impl TableTestFixture {
- fn new() -> Self {
- let tmp_dir = TempDir::new().unwrap();
- let table_location = tmp_dir.path().join("table1");
- let manifest_list1_location = table_location.join("metadata/manifests_list_1.avro");
- let manifest_list2_location = table_location.join("metadata/manifests_list_2.avro");
- let table_metadata1_location = table_location.join("metadata/v1.json");
-
- let file_io = FileIO::from_path(table_location.as_os_str().to_str().unwrap())
- .unwrap()
- .build()
- .unwrap();
-
- let table_metadata = {
- let template_json_str = fs::read_to_string(format!(
- "{}/testdata/example_table_metadata_v2.json",
- env!("CARGO_MANIFEST_DIR")
- ))
- .unwrap();
- let mut context = Context::new();
- context.insert("table_location", &table_location);
- context.insert("manifest_list_1_location", &manifest_list1_location);
- context.insert("manifest_list_2_location", &manifest_list2_location);
- context.insert("table_metadata_1_location", &table_metadata1_location);
-
- let metadata_json = Tera::one_off(&template_json_str, &context, false).unwrap();
- serde_json::from_str::<TableMetadata>(&metadata_json).unwrap()
- };
-
- let table = Table::builder()
- .metadata(table_metadata)
- .identifier(TableIdent::from_strs(["db", "table1"]).unwrap())
- .file_io(file_io)
- .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap())
- .build();
-
- Self {
- table_location: table_location.to_str().unwrap().to_string(),
- table,
- }
- }
-
- fn next_manifest_file(&self) -> OutputFile {
- self.table
- .file_io()
- .new_output(format!(
- "{}/metadata/manifest_{}.avro",
- self.table_location,
- Uuid::new_v4()
- ))
- .unwrap()
- }
-
- async fn setup_manifest_files(&mut self) {
- let current_snapshot = self.table.metadata().current_snapshot().unwrap();
- let parent_snapshot = current_snapshot
- .parent_snapshot(self.table.metadata())
- .unwrap();
- let current_schema = current_snapshot.schema(self.table.metadata()).unwrap();
- let current_partition_spec = self.table.metadata().default_partition_spec().unwrap();
-
- // Write data files
- let data_file_manifest = ManifestWriter::new(
- self.next_manifest_file(),
- current_snapshot.snapshot_id(),
- vec![],
- )
- .write(Manifest::new(
- ManifestMetadata::builder()
- .schema((*current_schema).clone())
- .content(ManifestContentType::Data)
- .format_version(FormatVersion::V2)
- .partition_spec((**current_partition_spec).clone())
- .schema_id(current_schema.schema_id())
- .build(),
- vec![
- ManifestEntry::builder()
- .status(ManifestStatus::Added)
- .data_file(
- DataFileBuilder::default()
- .content(DataContentType::Data)
- .file_path(format!("{}/1.parquet", &self.table_location))
- .file_format(DataFileFormat::Parquet)
- .file_size_in_bytes(100)
- .record_count(1)
- .partition(Struct::from_iter([Some(Literal::long(100))]))
- .build()
- .unwrap(),
- )
- .build(),
- ManifestEntry::builder()
- .status(ManifestStatus::Deleted)
- .snapshot_id(parent_snapshot.snapshot_id())
- .sequence_number(parent_snapshot.sequence_number())
- .file_sequence_number(parent_snapshot.sequence_number())
- .data_file(
- DataFileBuilder::default()
- .content(DataContentType::Data)
- .file_path(format!("{}/2.parquet", &self.table_location))
- .file_format(DataFileFormat::Parquet)
- .file_size_in_bytes(100)
- .record_count(1)
- .partition(Struct::from_iter([Some(Literal::long(200))]))
- .build()
- .unwrap(),
- )
- .build(),
- ManifestEntry::builder()
- .status(ManifestStatus::Existing)
- .snapshot_id(parent_snapshot.snapshot_id())
- .sequence_number(parent_snapshot.sequence_number())
- .file_sequence_number(parent_snapshot.sequence_number())
- .data_file(
- DataFileBuilder::default()
- .content(DataContentType::Data)
- .file_path(format!("{}/3.parquet", &self.table_location))
- .file_format(DataFileFormat::Parquet)
- .file_size_in_bytes(100)
- .record_count(1)
- .partition(Struct::from_iter([Some(Literal::long(300))]))
- .build()
- .unwrap(),
- )
- .build(),
- ],
- ))
- .await
- .unwrap();
+/// Evaluates manifest files to see if their partition values comply with a filter predicate
+pub struct PartitionEvaluator {
+ manifest_eval_visitor: ManifestEvalVisitor,
Review Comment:
I don't think we should nest `ManifestEvalVisitor` inside `PartitionEvaluator` since both structs have separate responsibilities and could exists independently (I shouldn't need a ManifestEvalVisitor to do partition pruning).
Also I think the implementation of `PartitionEvaluator` is missing [[reference](https://github.com/apache/iceberg-python/blob/main/pyiceberg/expressions/visitors.py#L468)]?
It should visit each `DataFile`s struct and evaluate the predicate.
##########
crates/iceberg/src/scan.rs:
##########
@@ -214,326 +272,559 @@ impl FileScanTask {
}
}
-#[cfg(test)]
-mod tests {
- use crate::io::{FileIO, OutputFile};
- use crate::spec::{
- DataContentType, DataFileBuilder, DataFileFormat, FormatVersion, Literal, Manifest,
- ManifestContentType, ManifestEntry, ManifestListWriter, ManifestMetadata, ManifestStatus,
- ManifestWriter, Struct, TableMetadata, EMPTY_SNAPSHOT_ID,
- };
- use crate::table::Table;
- use crate::TableIdent;
- use arrow_array::{ArrayRef, Int64Array, RecordBatch};
- use futures::TryStreamExt;
- use parquet::arrow::{ArrowWriter, PARQUET_FIELD_ID_META_KEY};
- use parquet::basic::Compression;
- use parquet::file::properties::WriterProperties;
- use std::collections::HashMap;
- use std::fs;
- use std::fs::File;
- use std::sync::Arc;
- use tempfile::TempDir;
- use tera::{Context, Tera};
- use uuid::Uuid;
-
- struct TableTestFixture {
- table_location: String,
- table: Table,
- }
-
- impl TableTestFixture {
- fn new() -> Self {
- let tmp_dir = TempDir::new().unwrap();
- let table_location = tmp_dir.path().join("table1");
- let manifest_list1_location = table_location.join("metadata/manifests_list_1.avro");
- let manifest_list2_location = table_location.join("metadata/manifests_list_2.avro");
- let table_metadata1_location = table_location.join("metadata/v1.json");
-
- let file_io = FileIO::from_path(table_location.as_os_str().to_str().unwrap())
- .unwrap()
- .build()
- .unwrap();
-
- let table_metadata = {
- let template_json_str = fs::read_to_string(format!(
- "{}/testdata/example_table_metadata_v2.json",
- env!("CARGO_MANIFEST_DIR")
- ))
- .unwrap();
- let mut context = Context::new();
- context.insert("table_location", &table_location);
- context.insert("manifest_list_1_location", &manifest_list1_location);
- context.insert("manifest_list_2_location", &manifest_list2_location);
- context.insert("table_metadata_1_location", &table_metadata1_location);
-
- let metadata_json = Tera::one_off(&template_json_str, &context, false).unwrap();
- serde_json::from_str::<TableMetadata>(&metadata_json).unwrap()
- };
-
- let table = Table::builder()
- .metadata(table_metadata)
- .identifier(TableIdent::from_strs(["db", "table1"]).unwrap())
- .file_io(file_io)
- .metadata_location(table_metadata1_location.as_os_str().to_str().unwrap())
- .build();
-
- Self {
- table_location: table_location.to_str().unwrap().to_string(),
- table,
- }
- }
-
- fn next_manifest_file(&self) -> OutputFile {
- self.table
- .file_io()
- .new_output(format!(
- "{}/metadata/manifest_{}.avro",
- self.table_location,
- Uuid::new_v4()
- ))
- .unwrap()
- }
-
- async fn setup_manifest_files(&mut self) {
- let current_snapshot = self.table.metadata().current_snapshot().unwrap();
- let parent_snapshot = current_snapshot
- .parent_snapshot(self.table.metadata())
- .unwrap();
- let current_schema = current_snapshot.schema(self.table.metadata()).unwrap();
- let current_partition_spec = self.table.metadata().default_partition_spec().unwrap();
-
- // Write data files
- let data_file_manifest = ManifestWriter::new(
- self.next_manifest_file(),
- current_snapshot.snapshot_id(),
- vec![],
- )
- .write(Manifest::new(
- ManifestMetadata::builder()
- .schema((*current_schema).clone())
- .content(ManifestContentType::Data)
- .format_version(FormatVersion::V2)
- .partition_spec((**current_partition_spec).clone())
- .schema_id(current_schema.schema_id())
- .build(),
- vec![
- ManifestEntry::builder()
- .status(ManifestStatus::Added)
- .data_file(
- DataFileBuilder::default()
- .content(DataContentType::Data)
- .file_path(format!("{}/1.parquet", &self.table_location))
- .file_format(DataFileFormat::Parquet)
- .file_size_in_bytes(100)
- .record_count(1)
- .partition(Struct::from_iter([Some(Literal::long(100))]))
- .build()
- .unwrap(),
- )
- .build(),
- ManifestEntry::builder()
- .status(ManifestStatus::Deleted)
- .snapshot_id(parent_snapshot.snapshot_id())
- .sequence_number(parent_snapshot.sequence_number())
- .file_sequence_number(parent_snapshot.sequence_number())
- .data_file(
- DataFileBuilder::default()
- .content(DataContentType::Data)
- .file_path(format!("{}/2.parquet", &self.table_location))
- .file_format(DataFileFormat::Parquet)
- .file_size_in_bytes(100)
- .record_count(1)
- .partition(Struct::from_iter([Some(Literal::long(200))]))
- .build()
- .unwrap(),
- )
- .build(),
- ManifestEntry::builder()
- .status(ManifestStatus::Existing)
- .snapshot_id(parent_snapshot.snapshot_id())
- .sequence_number(parent_snapshot.sequence_number())
- .file_sequence_number(parent_snapshot.sequence_number())
- .data_file(
- DataFileBuilder::default()
- .content(DataContentType::Data)
- .file_path(format!("{}/3.parquet", &self.table_location))
- .file_format(DataFileFormat::Parquet)
- .file_size_in_bytes(100)
- .record_count(1)
- .partition(Struct::from_iter([Some(Literal::long(300))]))
- .build()
- .unwrap(),
- )
- .build(),
- ],
- ))
- .await
- .unwrap();
+/// Evaluates manifest files to see if their partition values comply with a filter predicate
+pub struct PartitionEvaluator {
+ manifest_eval_visitor: ManifestEvalVisitor,
+}
- // Write to manifest list
- let mut manifest_list_write = ManifestListWriter::v2(
- self.table
- .file_io()
- .new_output(current_snapshot.manifest_list())
- .unwrap(),
- current_snapshot.snapshot_id(),
- current_snapshot
- .parent_snapshot_id()
- .unwrap_or(EMPTY_SNAPSHOT_ID),
- current_snapshot.sequence_number(),
- );
- manifest_list_write
- .add_manifests(vec![data_file_manifest].into_iter())
- .unwrap();
- manifest_list_write.close().await.unwrap();
-
- // prepare data
- let schema = {
- let fields =
- vec![
- arrow_schema::Field::new("col", arrow_schema::DataType::Int64, true)
- .with_metadata(HashMap::from([(
- PARQUET_FIELD_ID_META_KEY.to_string(),
- "0".to_string(),
- )])),
- ];
- Arc::new(arrow_schema::Schema::new(fields))
- };
- let col = Arc::new(Int64Array::from_iter_values(vec![1; 1024])) as ArrayRef;
- let to_write = RecordBatch::try_new(schema.clone(), vec![col]).unwrap();
-
- // Write the Parquet files
- let props = WriterProperties::builder()
- .set_compression(Compression::SNAPPY)
- .build();
-
- for n in 1..=3 {
- let file = File::create(format!("{}/{}.parquet", &self.table_location, n)).unwrap();
- let mut writer =
- ArrowWriter::try_new(file, to_write.schema(), Some(props.clone())).unwrap();
-
- writer.write(&to_write).expect("Writing batch");
-
- // writer must be closed to write footer
- writer.close().unwrap();
- }
- }
+impl PartitionEvaluator {
+ pub(crate) fn new(
+ partition_spec: PartitionSpecRef,
+ partition_filter: BoundPredicate,
+ table_schema: SchemaRef,
+ ) -> crate::Result<Self> {
+ let manifest_eval_visitor = ManifestEvalVisitor::manifest_evaluator(
+ partition_spec,
+ table_schema,
+ partition_filter,
+ true,
+ )?;
+
+ Ok(PartitionEvaluator {
+ manifest_eval_visitor,
+ })
}
- #[test]
- fn test_table_scan_columns() {
- let table = TableTestFixture::new().table;
-
- let table_scan = table.scan().select(["x", "y"]).build().unwrap();
- assert_eq!(vec!["x", "y"], table_scan.column_names);
-
- let table_scan = table
- .scan()
- .select(["x", "y"])
- .select(["z"])
- .build()
- .unwrap();
- assert_eq!(vec!["z"], table_scan.column_names);
+ pub(crate) fn filter_manifest_file(&self, _manifest_file: &ManifestFile) -> bool {
+ self.manifest_eval_visitor.eval(_manifest_file)
}
+}
- #[test]
- fn test_select_all() {
- let table = TableTestFixture::new().table;
+struct ManifestEvalVisitor {
+ #[allow(dead_code)]
+ partition_schema: SchemaRef,
+ partition_filter: BoundPredicate,
+ #[allow(dead_code)]
+ case_sensitive: bool,
+}
- let table_scan = table.scan().select_all().build().unwrap();
- assert!(table_scan.column_names.is_empty());
+impl ManifestEvalVisitor {
+ fn new(
+ partition_schema: SchemaRef,
+ partition_filter: Predicate,
+ case_sensitive: bool,
+ ) -> crate::Result<Self> {
+ let partition_filter = partition_filter.bind(partition_schema.clone(), case_sensitive)?;
+
+ Ok(Self {
+ partition_schema,
+ partition_filter,
+ case_sensitive,
+ })
}
- #[test]
- fn test_select_no_exist_column() {
- let table = TableTestFixture::new().table;
-
- let table_scan = table.scan().select(["x", "y", "z", "a"]).build();
- assert!(table_scan.is_err());
+ pub(crate) fn manifest_evaluator(
Review Comment:
Perhaps we can combine this with `fn new` and get rid of `fn manifest_evaluator`. Although it aligns with the other implementations, it feels kinda strange - and I think using `ManifestEvalVisitor::new()` and then `.project()` is better?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1528775973
##########
crates/iceberg/src/scan.rs:
##########
@@ -214,11 +273,236 @@ impl FileScanTask {
}
}
+/// Evaluates manifest files to see if their partition values comply with a filter predicate
+pub struct PartitionEvaluator {
+ manifest_eval_visitor: ManifestEvalVisitor,
+}
+
+impl PartitionEvaluator {
+ pub(crate) fn new(
+ partition_spec: PartitionSpecRef,
+ partition_filter: BoundPredicate,
+ table_schema: SchemaRef,
+ ) -> crate::Result<Self> {
+ let manifest_eval_visitor = ManifestEvalVisitor::manifest_evaluator(
+ partition_spec,
+ table_schema,
+ partition_filter,
+ true,
+ )?;
+
+ Ok(PartitionEvaluator {
+ manifest_eval_visitor,
+ })
+ }
+
+ pub(crate) fn filter_manifest_file(&self, _manifest_file: &ManifestFile) -> bool {
+ self.manifest_eval_visitor.eval(_manifest_file)
+ }
+}
+
+struct ManifestEvalVisitor {
+ partition_schema: SchemaRef,
+ partition_filter: BoundPredicate,
+ case_sensitive: bool,
+}
+
+impl ManifestEvalVisitor {
+ fn new(
+ partition_schema: SchemaRef,
+ partition_filter: Predicate,
+ case_sensitive: bool,
+ ) -> crate::Result<Self> {
+ let partition_filter = partition_filter.bind(partition_schema.clone(), case_sensitive)?;
+
+ Ok(Self {
+ partition_schema,
+ partition_filter,
+ case_sensitive,
+ })
+ }
+
+ pub(crate) fn manifest_evaluator(
+ partition_spec: PartitionSpecRef,
+ table_schema: SchemaRef,
+ partition_filter: BoundPredicate,
+ case_sensitive: bool,
+ ) -> crate::Result<Self> {
+ let partition_type = partition_spec.partition_type(&table_schema)?;
+
+ // this is needed as SchemaBuilder.with_fields expects an iterator over
+ // Arc<NestedField> rather than &Arc<NestedField>
+ let cloned_partition_fields: Vec<_> = partition_type.fields().iter().map(Arc::clone).collect();
+
+ let partition_schema = Schema::builder()
+ .with_fields(cloned_partition_fields)
+ .build()?;
+
+ let partition_schema_ref = Arc::new(partition_schema);
+
+ let inclusive_projection =
+ InclusiveProjection::new(table_schema.clone(), partition_spec.clone());
+ let unbound_partition_filter = inclusive_projection.project(&partition_filter);
+
+ Ok(Self::new(
+ partition_schema_ref.clone(),
+ unbound_partition_filter,
+ case_sensitive,
+ )?)
+ }
+
+ pub(crate) fn eval(&self, manifest_file: &ManifestFile) -> bool {
+ if manifest_file.partitions.is_empty() {
+ return true;
+ }
+
+ self.visit(&self.partition_filter, &manifest_file.partitions)
+ }
+
+ // see https://github.com/apache/iceberg-python/blob/ea9da8856a686eaeda0d5c2be78d5e3102b67c44/pyiceberg/expressions/visitors.py#L548
+ fn visit(&self, predicate: &BoundPredicate, partitions: &Vec<FieldSummary>) -> bool {
+ match predicate {
+ AlwaysTrue => true,
+ BoundPredicate::AlwaysFalse => false,
+ BoundPredicate::And(expr) => {
+ self.visit(expr.inputs()[0], partitions) && self.visit(expr.inputs()[1], partitions)
Review Comment:
Maybe we can have something like `fn eval(&self, Vec<Literal>)->bool` for the `Predicate`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1547190841
##########
crates/iceberg/src/scan.rs:
##########
@@ -155,8 +189,22 @@ impl TableScan {
.await?;
// Generate data file stream
- let mut entries = iter(manifest_list.entries());
- while let Some(entry) = entries.next().await {
+ for entry in manifest_list.entries() {
+ // If this scan has a filter, check the partition evaluator cache for an existing
+ // PartitionEvaluator 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) = self.filter.as_ref() {
+ let partition_evaluator = partition_evaluator_cache
+ .entry(entry.partition_spec_id())
+ .or_insert_with_key(|key| self.create_partition_evaluator(key, filter));
+
+ // reject any manifest files whose partition values don't match the filter.
+ if !partition_evaluator.filter_manifest_file(entry) {
Review Comment:
I think we should apply `ManifestEvaluator` independently from `PartitionEvaluator` (see comment down below in struct PartitionEvaluator).
Also, I'm not sure if we can simply 'skip' the file with continue. In the python impl the Partition- and MetricsEvaluator are applied on each DataFile on multiple threads - so I believe we have to collect all relevant manifest files before and then apply the Partition- and MetricsEvaluator on the complete (pre-filtered) collection (or split the work into multiple threads)?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `PartitionEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-1987748406
Sounds good @liurenjie1024 - will do.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2044262473
Closing this PR as it has been split into https://github.com/apache/iceberg-rust/pull/317, https://github.com/apache/iceberg-rust/pull/320, https://github.com/apache/iceberg-rust/pull/321, https://github.com/apache/iceberg-rust/pull/322, and https://github.com/apache/iceberg-rust/pull/323
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2016630719
@liurenjie1024 are you able to take a look at this again please when you have time? I've resolved the outstanding issues with binding and added accessors to address the missing functionality.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2031946776
> @liurenjie1024 @ZENOTME @sdd @Xuanwo I'd really appreciate your thoughts on this:
>
> I took a closer look at the work @sdd has already done - and I think in order to proceed it would make sense to split the `ManifestEvaluator`, `InclusiveProjection` and `PartitionEvaluator` into separate modules.
>
> I am thinking about putting each visitor into its own file within `/iceberg/src/expr` (perhaps even another subfolder /visitors) - for example: `/iceberg/src/expr/manifest_evaluator.rs.`
>
> This way, we could create individual issues on each implementation - and work better in parallel.
>
> Perhaps, it also makes sense to provide a trait to make sure each visitor adheres to the same interface (although I'm not sure this is neccessary right now...)
>
> ```rust
> // Pseudo-Example
> pub trait BooleanVisitor:
> fn eval() -> boolean
> ```
+1 for splitting these into split modules. Instead of a boolean visitor, I'm thinking about a post order predicate visitor:
```rust
pub trait BoundPredicateVisitor {
}
```
which is somehow motivated by @viirya 's [pr](https://github.com/apache/iceberg-rust/pull/295/files#diff-a59622727cd67153abdf02031475bf8a1b1921738df4ca9903a685ff6970b7aaR472), but move the travsering flow out of trait body.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `ManifestEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1550859891
##########
crates/iceberg/src/expr/visitors/bound_predicate_evaluator.rs:
##########
@@ -0,0 +1,339 @@
+// 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::{BoundPredicate, BoundReference, PredicateOperator};
+use crate::spec::Datum;
+use crate::Result;
+use fnv::FnvHashSet;
+
+pub trait BoundPredicateEvaluator {
Review Comment:
cc @liurenjie1024
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `PartitionEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on code in PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#discussion_r1520319219
##########
crates/iceberg/src/scan.rs:
##########
@@ -52,6 +62,18 @@ impl<'a> TableScanBuilder<'a> {
self
}
+ /// Sets the scan as being case-insensitive
+ pub fn with_case_insensitivity(mut self) -> Self {
Review Comment:
I'm open to that. I assumed that being case-sensitive would be the default, and so you'd only need to call the builder method if you wanted the opposite of that. Keen to hear other thoughts.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WIP] Add `PartitionEvaluator` to allow filtering of files in a table scan (Issue #152) [iceberg-rust]
Posted by "sdd (via GitHub)" <gi...@apache.org>.
sdd commented on PR #241:
URL: https://github.com/apache/iceberg-rust/pull/241#issuecomment-2002482131
@liurenjie1024 @Xuanwo @ZENOTME: I've updated this PR, details in the updated description at the top. It builds on the `Bind` implementation that recently got merged and is all set to make use of the `Transform::project` implementation in https://github.com/apache/iceberg-rust/pull/269.
There are a few issues that I could do with some help on how best to get them resolved, mentioned in the "Unsolved Problems" section of the description at the top if 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org