chore: fix IsBoundVisitor to error on AlwaysTrue & AlwaysFalse#503
chore: fix IsBoundVisitor to error on AlwaysTrue & AlwaysFalse#503wgtmac merged 2 commits intoapache:mainfrom
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
src/iceberg/expression/binder.cc
Outdated
|
|
||
| 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.
zhjwpku
force-pushed
the
fix_is_bound_expression
branch
from
2eeefec to
a38bea7
Compare
wgtmac
changed the title
2 participants
No description provided.