API: Extract superclass from InclusiveMetricsEvaluator

[rdblue]

This extracts the evaluation logic from InclusiveMetricsEvaluator into a new superclass, InclusiveEvalVisitor. Now, InclusiveMetricsEvaluator uses the visitor superclass and implements methods to access metrics by ID. This is in preparation for adding an inclusive stats visitor that consumes ContentStats directly, once #17159 is in.

[anoopj]

There is a subtle behavior change here:

the previous version of uniqueValue returned null only if there are known NaN values. ie:

     if (nanCounts != null && nanCounts.containsKey(id) && nanCounts.get(id) != 0) {
        return null;
      }

But this version does so if the stats are missing (e.g. a if the column type is string. This is likely causing CI failures like below:

TestSparkScan > testUnpartitionedYears() > catalogName = testhadoop, implementation = org.apache.iceberg.spark.SparkCatalog, config = {type=hadoop, cache-enabled=false}, format = parquet FAILED

    java.lang.AssertionError: 

    Expected size: 5 but was: 10 in:

    [org.apache.iceberg.spark.source.SparkInputPartition@[...]

        at org.apache.iceberg.spark.source.TestSparkScan.testUnpartitionedYears(TestSparkScan.java:496)

Did you mean to make this change? The PR description says refactor.

[rdblue]

I fixed this by adding a check whether lower is a float or double.

The logic here was wrong. If the type is a floating point then this should be triggered by mayContainNaN because a NaN is not equal to the lower or upper bound. However, you're right that this is causing a failure because other types don't store NaN counts and so it is assumed a NaN is possible. Restricting this logic to just cases when lower is a floating point works as an easy way to test the type. We know that lower is not null or NaN from the check above.

This was a better place to check the type than in mayContainNaN to avoid checking types in the NaN methods. Normally, those are only called for isNaN and notNaN evaluations, where we already know the field is a floating point type because the check is done in expression binding.

[nastra]

that new check makes sense to me, but since this is a pure structural refactoring PR maybe we should extract that fix into a separate PR and have a test for it? Otherwise it won't be obvious to readers of the code when trying to figure out when/how something changed (even if it actually fixes a bug)

[dramaticlly]

Agreed might worth a test in TestInclusiveMetricsEvaluator for float type without NaN value counts, recall there's previous discussion on the optimization PR when trying to prune for single values with not-in and != in, and I think the change did fix the gap from #14593 (comment)

[rdblue]

I don't think we need to separate this into a different PR. The change is a necessary part of this refactor because it needs to rely on mayContainNaN rather than a custom check.

I added a couple of correct tests for float and updated the string tests to fail correctly.

[nastra]

changes LGTM, but would be great to extract the correctness fix into a separate PR with a test

[rdblue]

Thanks for the reviews, @anoopj, @nastra, @dramaticlly, and @huaxingao! I'll merge this.