bodduv commented on code in PR #14500:
URL: https://github.com/apache/iceberg/pull/14500#discussion_r2792552064
##########
api/src/main/java/org/apache/iceberg/expressions/InclusiveMetricsEvaluator.java:
##########
@@ -86,8 +107,12 @@ private class MetricsEvalVisitor extends
ExpressionVisitors.BoundVisitor<Boolean
private Map<Integer, Long> nanCounts = null;
private Map<Integer, ByteBuffer> lowerBounds = null;
private Map<Integer, ByteBuffer> upperBounds = null;
+ // Flag to use signed UUID comparator for backward compatibility.
+ // This is needed for the IN predicate because the comparator information
is lost
+ // when binding converts literals to a Set<T> of raw values.
+ private boolean useSignedUuidComparator = false;
- private boolean eval(ContentFile<?> file) {
+ private boolean eval(ContentFile<?> file, boolean signedUuidMode) {
Review Comment:
> Do we need the boolean flag because the IN evaluation only deals with
literalSet of raw values (not Literal wrapper)? Wondering if we can fix that
first?
- I found adding the boolean arg the most simplest way without causing
drastic changes that could involve changes to public interfaces. I think the
change is fairly easy to understand and reason although it is not particularly
pleasing to look at.
- Do we really want to delay fixing this bug in order to refactor parts of
code (that could take considerable amount of time and effort) primarily to not
have a boolean argument? considering the amount of time and effort being put in
this PR, I would assume it could take a couple of months. I would be open to
work on it, but I don't see outweighing benefits.
- I would assign a higher priority to fixing the bug and address the
criticality of inconsistency of Java implementation compared to Go, C++, Rust
implementations as early as possible.
--
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]
