sdd commented on code in PR #320: URL: https://github.com/apache/iceberg-rust/pull/320#discussion_r1554441701
########## crates/iceberg/src/expr/visitors/bound_predicate_visitor.rs: ########## @@ -0,0 +1,366 @@ +// 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 BoundPredicateVisitor { + type T; + + fn always_true(&mut self) -> Result<Self::T>; + fn always_false(&mut self) -> Result<Self::T>; + + fn and(&mut self, lhs: Self::T, rhs: Self::T) -> Result<Self::T>; + fn or(&mut self, lhs: Self::T, rhs: Self::T) -> Result<Self::T>; + fn not(&mut self, inner: Self::T) -> Result<Self::T>; + + fn is_null(&mut self, reference: &BoundReference) -> Result<Self::T>; Review Comment: I also like the idea of going further and having an `Expression` enum that contains the logical operators plus `Predicate` above, with `Reference` from the above being an enum over bound or unbound. This allows visitors to potentially work over both bound and unbound expressions. ```rust pub enum Expression { // The top level would be Expression rather than Predicate. // The logical operators would live at this level. // Since logical operators such as And / Or / Not // are common between bound and unbound, some visitors // such as rewrite_not can be applied to both bound and // unbound expressions, preventing duplication and increasing // flexibility /// An expression always evaluates to true. AlwaysTrue, /// An expression always evaluates to false. AlwaysFalse, // Here we use a struct variant to eliminate the need // for a generic `LogicalExpression`, which did not // add much, also allowing us to refer to lhs and rhs // by name /// An expression combined by `AND`, for example, `a > 10 AND b < 20`. And { lhs: Box<Expression>, rhs: Box<Expression> }, /// An expression combined by `OR`, for example, `a > 10 OR b < 20`. Or { lhs: Box<Expression>, rhs: Box<Expression> }, /// An expression combined by `NOT`, for example, `NOT (a > 10)`. Not(Box<Expression>), /// A Predicate Predicate(Predicate), } pub enum Reference { // Only at the leaf level do we distinguish between // bound and unbound. This allows things like bind // and unbind to be implemented as a transforming visitor, // and avoids duplication of logic compared to having // Bound / Unbound being distinguished at a higher level // in the type hierarchy Bound(BoundReference), Unbound(UnboundReference), } pub struct UnboundReference { name: String, } pub struct BoundReference { column_name: String, field: NestedFieldRef, accessor: StructAccessor, } ``` We could use type alises such as `type BoundExpression = Expression` and have `bind` return a `BoundExpression` to prevent bound expressions being used where unbound ones are required and vice versa. But I'm digressing very far from the original comment now, sorry! 😆 -- 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
