feat(exprs): Adding BooleanExpressions and Predicates #91
Conversation
There was a problem hiding this comment.
@zeroshade First of all, sorry for the late review. This looks good! Not many comments since it is mostly a copy of PyIceberg :)
Can you go over the small nits and resolve the conflicts so we can get this in? Thanks! 🙌
| OpLT // LessThan | ||
| OpLTEQ // LessThanEqual | ||
| OpGT // GreaterThan | ||
| OpGTEQ // GreaterThanEqual | ||
| OpEQ // Equal | ||
| OpNEQ // NotEqual |
There was a problem hiding this comment.
WDYT? For consistency. Sometimes we abbreviate with NEQ. maybe it is good to be more explicit in all of them:
| OpLT // LessThan | |
| OpLTEQ // LessThanEqual | |
| OpGT // GreaterThan | |
| OpGTEQ // GreaterThanEqual | |
| OpEQ // Equal | |
| OpNEQ // NotEqual | |
| OpLessThan // LessThan | |
| OpLessThanOrEqual // LessThanEqual | |
| OpGreaterThan // GreaterThan | |
| OpGreaterThanOrEqual // GreaterThanEqual | |
| OpEqualTo // Equal | |
| OpNotEqualTo // NotEqual |
This also removes the need for the comments :)
There was a problem hiding this comment.
I'm now seeing predicates.go, if this is just internal, feel free to ignore the comment above.
There was a problem hiding this comment.
So the comment lines on those are for use with stringer to auto-generate the String() method for the enum values. By adding the -linecomment option it uses the comment as the string that gets returned instead of just creating a string from the value. e.g. OpLessThan.String() would return "OpLessThan", but by using -linecomment and adding // LessThan to it, it returns "LessThan".
Does that make sense?
There was a problem hiding this comment.
Yes, that makes sense. Thanks for the explanation 👍
| return (a.left.Equals(rhs.left) && a.right.Equals(rhs.right)) || | ||
| (a.left.Equals(rhs.right) && a.right.Equals(rhs.left)) |
There was a problem hiding this comment.
I'm fine with this, but I don't think it is exhaustive. For example:
NewAnd(a, b, c, d) becomes AndExpr(a, AndExpr(b, AndExpr(c, d)))
This would yield False:
AndExpr(a, AndExpr(b, AndExpr(c, d))).equals(AndExpr(d, AndExpr(b, AndExpr(c, a))))
But the expression is equivalent
| } | ||
|
|
||
| func (a AndExpr) Negate() BooleanExpression { | ||
| return NewOr(a.left.Negate(), a.right.Negate()) |
| case T: | ||
| return Optional[T]{Valid: true, Val: v} | ||
| } | ||
| panic("unexpected type returned for bound ref") |
There was a problem hiding this comment.
Do we want to include the unexpected type in the error message?
| } | ||
|
|
||
| if (ul.op == OpStartsWith || ul.op == OpNotStartsWith) && | ||
| !bound.Type().Equals(PrimitiveTypes.String) { |
There was a problem hiding this comment.
(Not)StartsWith can also be applied to Binary types
zeroshade
force-pushed
the
expr-definitions
branch
from
b5e818d
to
d7cffa8
Compare
|
Sorry for the late review, thanks for working on this @zeroshade 🙌 |
Building on #76, this PR implements the interfaces and basic implementations of BooleanExpressions, bound and unbound predicates, and a series of wrapper functions for convenience. It also adds tests for these structs. Where possible, type safety is attempted by using generics to try to enforce types and to reduce the need for type switching in subsequent PRs.