wgtmac commented on code in PR #956:
URL: https://github.com/apache/iceberg-cpp/pull/956#discussion_r4094758384
##########
src/iceberg/data/file_scan_task_reader.cc:
##########
@@ -164,10 +165,21 @@ class FileScanTaskReader::Impl {
"Data file size must not be negative: {}",
data_file->file_size_in_bytes);
+ auto filter = task.residual_filter();
+ if (filter) {
+ ICEBERG_ASSIGN_OR_RAISE(auto is_bound, IsBoundVisitor::IsBound(filter));
Review Comment:
A normal scan can pass `True` as its residual, and constants can also be
nested in `And`/`Or`. `IsBoundVisitor` currently returns an error for both
constants, so these expressions fail before reading.
Java avoids this because its `IsBoundVisitor` returns `null` for constants,
and `Binder.BindVisitor` handles constants directly. Please mirror that
behavior. Simply returning `true` for constants is not sufficient: for
`And(True, unbound)`, that would mark the whole expression as bound and leave
the unbound predicate unbound.
One concrete fix is a tri-state result:
```cpp
// IsBoundVisitor
AlwaysTrue() -> std::nullopt; // constants only
AlwaysFalse() -> std::nullopt;
combine(left, right):
if (!left) return right;
if (!right) return left;
if (*left != *right) return InvalidExpression("Found partially bound
expression");
return left;
```
Then `Binder::Bind()` handles constants and unbound predicates. Add tests
for `True`, `False`, `And(True, pred)`, and `Or(False, pred)`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
