API, Core: Make TestInclusiveMetricsEvaluator/TestInclusiveMetricsEvaluatorWithExtract/TestInclusiveMetricsEvaluatorWithTransforms extendable - #17430
Open
nastra wants to merge 4 commits into
Conversation
The single_value_nulls fixtures in testNotEqWithSingleValue and testNotInWithSingleValue reported 2 nulls for field 3, the "required" column. A required column cannot contain nulls, so no writer produces those metrics. Move both to the optional "some_empty" column, which keeps the case the tests cover: a file whose bounds hold a single value still has to be read when the column also contains nulls, because those nulls match != and NOT IN. The NOT IN fixture also reported a NaN count for a string column, which only float and double columns track.
The range_of_values and single_value fixtures reported a NaN count for field 3, a string column. Only float and double columns track NaN counts, which is why the single_value fixture in testNotEqWithSingleValue already passed null for them. Generated-by: Cursor
`FILE_6` in `TestInclusiveMetricsEvaluator` carried value and null counts for the
`required_address` and `optional_address` structs. Parquet produces metrics for leaf
columns only in `ParquetMetrics.MetricsVisitor.struct` and returns just the concatenation of
its children and never emits a `FieldMetrics` for the struct — so no Parquet file yields
those entries. They made `notNull("optional_address")` appear prunable when in practice it is not
nastra
force-pushed
the
extend-metrics-eval-tests
branch
from
July 30, 2026 09:33
9f9524d to
90461f0
Compare
…luatorWithExtract/TestInclusiveMetricsEvaluatorWithTransforms extendable
nastra
force-pushed
the
extend-metrics-eval-tests
branch
from
July 30, 2026 09:38
90461f0 to
60d7aaa
Compare
uros-b
reviewed
Jul 30, 2026
| // read optional_address and its nested fields | ||
| shouldRead = | ||
| new InclusiveMetricsEvaluator(NESTED_SCHEMA, isNull("optional_address")).eval(FILE_6); | ||
| shouldRead = shouldRead(NESTED_SCHEMA, isNull("optional_address"), file6()); |
uros-b
reviewed
Jul 30, 2026
| return asFile(SINGLE_FLOAT_VALUE_FILE_WITH_NAN); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") |
Member
There was a problem hiding this comment.
Nit: the 15 protected factory methods each delegate to the private asFile(DataFile), which performs an unchecked (F) dataFile cast. A subclass that extends the base with a non-DataFile type F (e.g. a ContentFile variant) and fails to override even one factory method will receive a runtime ClassCastException with no compile-time signal; the type parameter F has no bound, so the compiler cannot catch it. The @SuppressWarnings("unchecked") on asFile() is correct per convention, but the contract ("subclasses using a non-DataFile F must override every factory method and both shouldRead() overloads") is nowhere documented. A Javadoc block on the class and/or on asFile() noting this is the recommended fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This makes the three test classes extendible so that we can introduce separate test classes that use a content stats evaluator but run through the same set of tests
AI Disclosure