gortiz opened a new pull request, #10352: URL: https://github.com/apache/pinot/pull/10352
When we were working on #10183, two classes that were quite problematic were `IndexLoadingConfig` and `SegmentGeneratorConfig`. Both classes are usually build from a `TableConfig`, which they use to extract some information and do some simple conversions. This is one example of the friction between the user syntax model and the programmer model that was described in that issue, but that is not actually important in the context of this PR. What it is important is that these two classes are being heavily modified in tests. What I mean by that is that there are a lot of tests that instead of instanciating these classes by using a `TableConfig` or instead of create new instances each time a modification has to be done, there are tests that directly modify `IndexLoadingConfig` and `SegmentGeneratorConfig`. These modifications are done by directly setting some properties (for example, calling `setInvertedIndexColumns`) but also by indirectly modifying the mutable collections (for example, calling `getInvertedIndexColumns` and then adding or removing some elements). This means that a `IndexLoadingConfig` may have a reference to a `TableConfig` that says there are no inverted indexes while `IndexLoadingConfig.getInvertedIndexColumns` return a non empty collection. Even worse, there are some situations where incompatible configurations are created programatically. Ideally, these classes should either not exist or at least should be immutable. That would in fact improve the coverage the tests and, if written correctly, that could even make tests easier to read. But it is not feasible to do that refactor right now. What this PR does, instead, is to modify `IndexLoadingConfig` and `SegmentGeneratorConfig` in such a way that they return unmodificable collections. By doing that, tests that were indirectly modifying these two objects have been changed to call older or new modification methods. These changes should not change anything in the production code, as this code should not be doing these tricks. Changes on the PR are very repetitive and most of the changes have been created replacing code patterns in files. In some places manual changes had to be done and these places are the ones where it is more probable to introduce an error. Luckily, given that changes mostly affect the tests, if the CI indicates that tests are correct, we can confidently approve the PR*. * At least semantically, obviously reviewers may have their own opinions -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
