chore: IsBoundVisitor mixed bound/unbound predicate should error #503
Conversation
src/iceberg/expression/binder.cc
Outdated
| } | ||
|
|
||
| Result<bool> IsBoundVisitor::AlwaysTrue() { return true; } | ||
| Result<std::optional<bool>> IsBoundVisitor::AlwaysTrue() { return std::nullopt; } |
There was a problem hiding this comment.
I think the Java impl is overly complicated to return null by default and then a lot of places should handle null with different meanings.
There was a problem hiding this comment.
What about letting each visitors to handle this instead of complicating the return type?
zhjwpku
force-pushed
the
fix_is_bound_expression
branch
from
1ba9c7d to
366c586
Compare
zhjwpku
force-pushed
the
fix_is_bound_expression
branch
from
366c586 to
7730c88
Compare
zhjwpku
changed the title
|
|
||
| Result<bool> IsBoundVisitor::And(bool left_result, bool right_result) { | ||
| return left_result && right_result; | ||
| ICEBERG_PRECHECK(left_result == right_result, "Found partially bound expression"); |
There was a problem hiding this comment.
I don't think this is correct. If we want to keep the same behavior as the Java impl, we might need to return error for AlwaysTrue() and AlwaysFalse() and then revert changes to And and Or.
There was a problem hiding this comment.
But if we do that, mixed bound and unbound will be evaluated as false, is that expected?
There was a problem hiding this comment.
That is expected. IsBoundVisitor checks if all expressions are bound and returns false if any is unbound. This is used to check if the expression has called bind function along the tree.
2 participants
No description provided.