Are you missing a @JsonProperty getter for this?

added (also a couple of other missing this)

Please annotate with @Nullable.

removed entirely

Please mark these @Nullable too, just like the constructor parameters.

Or, apply the defaults you have in getUseIndex(), etc, in the constructors and make these fields primitives.

applied defaults in constructor

Please include @JsonInclude(JsonInclude.Include.NON_NULL) here, and in all other places FilterTuning appears as a json property, to avoid polluting jsonifications of filters with this experimental feature.

added to DimFilter getters

Is it intended that anything outside of computeTuning call this method? If not, please update the javadocs to tell people not to call it. Maybe mention that you would have made it protected, except that Filter is an interface, so you can't do that.

this method has been removed

Similar comment to computeTuning, it seems like this might only be called by shouldUseIndex.

Btw, what do you think about not having getManualTuning and computeTuning, and instead having a static function in Filters that provides a useful default impl for shouldUseIndex? So most Filters would do something like:

@Override
boolean shouldUseIndex(BitmapIndexSelector selector)
{
  return Filters.shouldUseIndex(this, selector, filterTuning);
}

It's not really more work or lines of code than overriding getManualTuning, which they'd have to do anyway with the current design. And it cuts 3 interface methods down to 1.

It would also be less spooky in terms of action-at-a-distance. It's nice to have methods defined in concrete classes rather than inherited from supertypes, since it makes it clearer to readers of the one class what is going on (without having to flip around as much between concrete impl and supertype).

This makes sense, I think the current structure is a an artifact of my original version of this where the work was done by QueryableIndexStorageAdapter, will refactor

done

This'll never be null, so it'd be better to return a primitive.

Same comment for the other two methods.

In general I assume you left the old constructors in to avoid a bunch of test churn. I think that's fine but could you please double check that they aren't being used in production code? (You got the one in InDimFilter.optimize that I was thinking of when I wrote this, but there are potentially others)

there were a few being created in calcite stuff, adjusted them to set null for tuning.

Sometimes the field is called manualFilterTuning and sometimes it's called filterTuning. Could you please pick one?

I believe it is consistently called filterTuning on DimFilter and manualFilterTuning on Filter implementations. This distinction is because everything in DimFilter is manual since it was provided by the json query, where as this is explictly the user set value on Filter implementations that was supplied by the DimFilter.

renamed all to filterTuning

map(DimensionSpec::getDimension)?

done

Should be @Nullable, along with the corresponding constructor parameter

I think I've updated all DimFilter implementations to have the appropriate things annotated with @Nullable

This method should be @Nullable

Everything about extractionFn: the field, constructor parameter, getter should be @Nullable

Constructor annotated @VisibleForTesting doesn't mean it can't be used in production code.

Yeah, this is true, but I was trying to avoid using these constructors in production code to minimize the chance of errors and make things more obvious.

I initially had it more like you are suggesting and was using some of the 'test' constructors where we were always passing null, but this comment suggested the way it is now and I agree, I think the production code explicitly passing in null for the filterTuning parameter makes it less ambiguous about if not having a tuning is intentional or not, without requiring a comment.

I think it's anyway strange that there is a chaining constructor and any code is not using an existing constructor with fewer parameters.

I think it would be better to create static factory methods in this situation. Some of them may have "InTest" suffix to strongly indicate that they are not for prod code.

I agree, but the reason I have a second set of constructors at all was so I didn't have to change hundreds of lines of test code to add a null parameter, so making a static method would sort of defeat the purpose of that because the lines would change anyway.

I do think this is worth doing, I'll add this to the ticket i create about refactoring DimFilter to also add useful test filter creation static methods, so we can also eliminate passing in null everywhere for filters without extractionFn.

mentioned making static test object creators to decouple test filters from json schema in #8256

And here.

Please annotate with @Nullable everything that is needed in this class.

Could call the old constructor

Could call the old constructor

Unnecessary wrapping, also in many other newly added constructors below in this PR

Please annotate with @Nullable everything that is needed in this class.

Whoa. This is cool.

I'd reword slightly. This method doesn't do anything by default anymore (it did in an earlier version). Maybe cut the last two sentences, and replace with:

Implementations of this methods typically consider a {@link FilterTuning} to make decisions about when to use an available index.

Oops, forgot about this, will fix this and add javadoc for Filters.shouldUseIndex and maybe link this one to that

Thanks, these new ones look good to me.

Could use a bit of javadocs as to its purpose ("standard" implementation of Filter.shouldUseIndex) and how it works (returns true if the filter supports indexes and if all underlying columns match the thresholds in filterTuning). Also that filterTuning is nullable.

Thanks, this looks good. Marking the filterTuning parameter @Nullable would be helpful too.

Thanks, this looks good. Marking the filterTuning parameter @Nullable would be helpful too.

extractionFn should be @Nullable

tuning should be @Nullable

Thanks for this refactoring

Optional: I think it's more semantic to put @Nullable immediately before the type so that @Nullable ExtractionFn is the "extended type" of the variable.

agree, done to all occurrences I found in the codebase

Before the sentence "Implementations of this methods...", please add

*
* If shouldUseFilter(selector) returns true, {@link #supportsBitmapIndex} must also return true when called with the
* same selector object.
*

as a separate paragraph.

added

There is significant repetition between all these Filter implementations. Did you research if there is a way to cross-cut the things, e. g. by somehow providing filterTuning externally, so that each Filter doesn't have to have it as a field? Or, maybe an abstract superclass could be extracted?

Same applies to extractionFn.

I totally agree here. Doing it externally seemed difficult because I wanted it to be per filter, but I did heavily consider making some sort of abstract class on top of DimFilter to handle extractionFn and filterTuning, but I think at this point would prefer to do as a follow-up so I will open an issue soon about refactoring DimFilter, and potentially merging Filter and DimFilter as well since there isn't a good reason for them to be split.

created #8256

Please use consistent naming: either "supportsBitmapIndex" and "shouldUseBitmapIndex", or both names without "Bitmap" part.