Python: Refactor expression hierarchy #5389
Conversation
| (base.Not(ExpressionA()), "Not(testexpra)"), | ||
| (base.And(ExpressionA(), ExpressionB()), "And(left=ExpressionA(), right=ExpressionB())"), | ||
| (base.Or(ExpressionA(), ExpressionB()), "Or(left=ExpressionA(), right=ExpressionB())"), | ||
| (base.Not(ExpressionA()), "Not(child=ExpressionA())"), |
There was a problem hiding this comment.
nit: since updating this file, maybe worth try simply LINE 220 to 245 as Sam suggested in https://github.com/apache/iceberg/pull/5362/files#r932825540
def test_ref_binding_case_sensitive(request):
schema = request.getfixturevalue("table_schema_simple")can be replaced by python
def test_ref_binding_case_sensitive(table_schema_simple):There was a problem hiding this comment.
I didn't make that change because I think it is easier to understand what's happening when we use less magic. Using getfixturevalue makes it clear that this is getting a fixture called "table_schema_simple". Otherwise where that comes from is magic and confusing (at least to me).
| return self | ||
|
|
||
|
|
||
| class UnboundTerm(Term[T], Unbound[BoundTerm[T]], ABC): |
There was a problem hiding this comment.
Nit / non-blocking: An Unbound[BoundTerm[T]] is somewhat of a confusing type signature (particularly coming more from the Java side).
However UnboundTerm itself is clear as day so that's not particularly a concern.
kbendick
left a comment
kbendick
left a comment
There was a problem hiding this comment.
The inclusion of Term makes a lot of sense to me (coming from more experience on the Java side). Overall this LGTM and is a welcomed update. 🙂
* Convert And, Or, and Not to dataclasses. * Refactor base expression types.
5 participants
This makes some minor changes to the expression hierarchy, mostly making the classes only implement what they need to. For example,
BoundandUnbounddon't need to carry a bound type. ATermhas an associated generic type instead. This also:case_sensitiveeverywhere