API: Extract superclass from StrictMetricsEvaluator - #17511
Conversation
Extract the strict evaluation logic from StrictMetricsEvaluator into a new superclass, StrictEvalVisitor. StrictMetricsEvaluator now uses the visitor superclass and implements methods to access metrics by ID. This is in preparation for adding a strict stats visitor that consumes ContentStats directly.
3ff17a6 to
05165a7
Compare
| * <p>Unlike {@link #mayContainNull(int)}, this returns false when a null count is unavailable for | ||
| * the field but is available for other fields. | ||
| */ | ||
| protected abstract boolean canContainNulls(int id); |
There was a problem hiding this comment.
There might be abstract-method contract asymmetry with the inclusive twin. StrictEvalVisitor declares 8 abstract methods; whereas the merged InclusiveEvalVisitor declares only 4. The two extra semantic methods (canContainNulls(int id) and canContainNaNs(int id)) carry a subtly stricter contract than their mayContain* counterparts: canContainNulls returns false when the nullCounts map exists but has no entry for the field (where mayContainNull returns true in that case), and canContainNaNs returns false whenever a NaN count is unavailable.
Can this create friction when the downstream v4 strict ContentStats evaluator is written, since its author must satisfy a materially larger contract than the corresponding inclusive visitor? Perhaps we should either add a comment to StrictEvalVisitor explaining why the strict visitor's contract diverges from InclusiveEvalVisitor's, or if the inclusive stats path will also need these finer-grained predicates then adding canContainNulls/canContainNaNs to InclusiveEvalVisitor now while both visitors are being actively designed.
WDYT? @nastra
There was a problem hiding this comment.
canContainNulls and canContainNaNs are only temporarily needed and will be removed with nastra@9ebe146 in a follow-up. I wanted to have this PR be a pure refactoring PR and then add nastra@9ebe146 on top of that, which then results in an API that aligns with the InclusiveEvalVisitor
anoopj
left a comment
anoopj
left a comment
There was a problem hiding this comment.
The refactor looks correct to me. One small comment.
| * Return true if the NaN count is known and non-zero, false otherwise. | ||
| * | ||
| * <p>Unlike {@link #mayContainNaN(int)}, this returns false when the NaN count is unavailable. | ||
| * NaN counts are not tracked for non-floating point fields or by early writers. |
There was a problem hiding this comment.
Nit: we can just remove this line, as it's describing the format behavior. I know we are going to remove it anyway, per the other comment thread.
3 participants
Similar to #17201 this extracts the strict evaluation logic from
StrictMetricsEvaluatorinto a new superclass. StrictMetricsEvaluator now uses the visitor superclass and implements methods to access metrics by ID. This is in preparation for adding a strict stats visitor that consumesContentStatsdirectly.