Adjust field config

[walterddr]

FieldConfig currently listed IndexType as singleton.
This is not true since a field can have multiple indexing methods.

[siddharthteotia]

Can we do this in a backward compatible manner so that existing users of non-list indexType in FieldConfig don't break ?

[siddharthteotia]

Can we do this in a backward compatible manner so that existing users of non-list indexType in FieldConfig don't break ?

Just noticed that constructor retain the original field and just adds the new field as nullable. In that case, this should be fine.

[richardstartin]

@walterddr could you add a test config with multiple indexes to testConfigs/ so TableConfigTest checks it can be deserialised (which is what is proving your change is backwards compatible with old configs, incidentally)?

[codecov-commenter]

Codecov Report

Merging #7631 (dea27e9) into master (d42e415) will decrease coverage by 6.34%.
The diff coverage is 83.07%.

❗ Current head dea27e9 differs from pull request most recent head 0a1ea20. Consider uploading reports for the commit 0a1ea20 to get more accurate results

@@             Coverage Diff              @@
##             master    #7631      +/-   ##
============================================
- Coverage     71.60%   65.26%   -6.35%     
- Complexity     3942     3996      +54     
============================================
  Files          1562     1529      -33     
  Lines         79465    78057    -1408     
  Branches      11766    11635     -131     
============================================
- Hits          56903    50943    -5960     
- Misses        18724    23494    +4770     
+ Partials       3838     3620     -218     

Flag	Coverage Δ
integration1	?
integration2	?
unittests1	68.72% <83.04%> (+0.14%)	⬆️
unittests2	14.54% <1.03%> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ot/controller/tuner/NoOpTableTableConfigTuner.java	100.00% <ø> (ø)
...pinot/controller/tuner/RealTimeAutoIndexTuner.java	100.00% <ø> (ø)
...y/aggregation/function/AvgAggregationFunction.java	92.00% <ø> (-5.37%)	⬇️
...aggregation/function/CountAggregationFunction.java	59.52% <ø> (-26.53%)	⬇️
...regation/function/DistinctAggregationFunction.java	64.28% <ø> (+2.21%)	⬆️
...ion/function/DistinctCountAggregationFunction.java	31.39% <ø> (-0.31%)	⬇️
...nction/DistinctCountBitmapAggregationFunction.java	26.42% <ø> (-28.73%)	⬇️
.../function/DistinctCountHLLAggregationFunction.java	44.57% <ø> (-0.34%)	⬇️
...nction/DistinctCountRawHLLAggregationFunction.java	100.00% <ø> (+4.54%)	⬆️
...n/DistinctCountThetaSketchAggregationFunction.java	64.11% <ø> (-1.13%)	⬇️
... and 439 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d42e415...0a1ea20. Read the comment docs.

[walterddr]

Just noticed that constructor retain the original field and just adds the new field as nullable. In that case, this should be fine.

@siddharthteotia: yes. jackson has this limitation that only one @JsonCreator annotated constructor can be used. i dunno if there's any better way.

@walterddr could you add a test config with multiple indexes to testConfigs/ so TableConfigTest checks it can be deserialised (which is what is proving your change is backwards compatible with old configs, incidentally)?

I already added a multiple indexes FieldConfigs in TestSerde. did you mean I should also add a fieldConfigList item in the backward-compatibility test cases, e.g. in compatibility-verifier/sample-test-suite/config/feature-test-1.json?

[richardstartin]

I already added a multiple indexes FieldConfigs in TestSerde. did you mean I should also add a fieldConfigList item in the backward-compatibility test cases, e.g. in compatibility-verifier/sample-test-suite/config/feature-test-1.json?

No, I meant add a test config with multiple indexes to testConfigs/ so TableConfigTest (introduced in #7615) checks it can be deserialised. The existing testing strategy did not find the defect fixed on that PR.

[walterddr]

No, I meant add a test config with multiple indexes to testConfigs/ so TableConfigTest (introduced in #7615) checks it can be deserialised. The existing testing strategy did not find the defect fixed on that PR.

Ahh. Didn't rebase against your PR. let me rebase and add a test case

approval was by mistake

[Jackie-Jiang]

We don't want to have a single element null as _indexTypes when both indexType and indexTypes are null.
I'd recommend having _indexTypes never null, but can be empty.

[Jackie-Jiang]

LGTM

[Jackie-Jiang]

(optional, code format) keep parameters in the same line. Same for the other constructor