Harmonization and bug-fixing for selector and filter behavior on unknown types. #9484
Conversation
…own types. - Migrate ValueMatcherColumnSelectorStrategy to newer ColumnProcessorFactory system, and set defaultType COMPLEX so unknown types can be dynamically matched. - Remove ValueGetters in favor of ColumnComparisonFilter doing its own thing. - Switch various methods to use convertObjectToX when casting to numbers, rather than ad-hoc and inconsistent logic. - Fix bug in RowBasedExpressionColumnValueSelector: isBindingArray should return true even for 0- or 1- element arrays. - Adjust various javadocs.
|
Note to reviewers — some of the bugs fixed here aren't tested by existing tests, but I plan to add tests for them in a future patch that also adds a RowBasedStorageAdapter. That's because the simplest & best way to test them is to add a row-based cursor to BaseFilterTest, which won't exist until the future patch. |
| @@ -95,7 +95,7 @@ private boolean isBindingArray(String x) | |||
| { | |||
| Object binding = bindings.get(x); | |||
| if (binding != null) { | |||
| if (binding instanceof String[] && ((String[]) binding).length > 1) { | |||
| if (binding instanceof String[]) { | |||
| }; | ||
| } | ||
|
|
||
| public static ValueMatcher makeLongValueMatcher(final BaseLongColumnValueSelector selector, final String value) |
There was a problem hiding this comment.
super nit: missing javadoc (since almost all the others have it)
| }; | ||
| } | ||
|
|
||
| public static ValueMatcher makeLongValueMatcher( |
|
Pushed some updates to address test failures in InputRowSerdeTest. I had to add a |
|
Tagged release notes because this PR changes the behavior of complex metric aggregation at ingestion time when SQL compatible null handling is disabled (the default mode) to now aggregate the default 0 values for rows instead of skipping them. This change is for the better imo since it makes things symmetrical to as if you ingested the raw data and built the sketch at query time, but it is different so worth calling out, and you can see the effects in some of the test changes in this PR. |
Thanks for pointing that out. Yes, I agree, it is for the better since it makes the ingest-time behavior and query-time behavior the same. This is part of the promise of Druid rollup in the first place (you can move aggregations to ingest time if you want). Btw, this patch also ends up making ingest-time transforms and filters behave more consistently with query-time ones. The reason is that all this ingest-time stuff runs in unknown-type mode, which til now had various inconsistencies with known-type mode (which is used at query time). |
system, and set defaultType COMPLEX so unknown types can be dynamically matched.
than ad-hoc and inconsistent logic.
true even for 0- or 1- element arrays.