badalprasadsingh commented on code in PR #2513:
URL: https://github.com/apache/iceberg-go/pull/2513#discussion_r4541110723
##########
table/evaluators.go:
##########
@@ -74,6 +74,12 @@ func (m *manifestEvalVisitor) Eval(manifest
iceberg.ManifestFile) (bool, error)
if len(parts) == 0 {
return rowsMightMatch, nil
}
+ for _, part := range parts {
+ if (part.LowerBound == nil) != (part.UpperBound == nil) {
+ // One bound without the other cannot prove the
manifest has no matches.
+ return rowsMightMatch, nil
+ }
+ }
Review Comment:
```go
for _, part := range parts {
noValues := part.LowerBound == nil && !part.ContainsNull &&
(part.ContainsNaN == nil || !*part.ContainsNaN)
if noValues || (part.LowerBound == nil) != (part.UpperBound == nil) {
return rowsMightMatch, nil
}
}
```
##########
table/evaluators.go:
##########
@@ -74,6 +74,12 @@ func (m *manifestEvalVisitor) Eval(manifest
iceberg.ManifestFile) (bool, error)
if len(parts) == 0 {
return rowsMightMatch, nil
}
+ for _, part := range parts {
+ if (part.LowerBound == nil) != (part.UpperBound == nil) {
+ // One bound without the other cannot prove the
manifest has no matches.
+ return rowsMightMatch, nil
+ }
+ }
Review Comment:
P0: if a manifest holds only "", go writes no bounds and contains_null =
false (#2407), merge-on-read deletes in "" partitions get such manifests, the
PR reads the data but skips those deletes, so col = '' returns deleted rows
better: keep them here, drop the dynamicOverwriteManifestCandidates loop,
add tests
--
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]
