Do not allow setting TableConfig in IndexLoadingConfig after construction #14098
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14098 +/- ##
============================================
+ Coverage 61.75% 64.03% +2.28%
- Complexity 207 1537 +1330
============================================
Files 2436 2596 +160
Lines 133233 143341 +10108
Branches 20636 21956 +1320
============================================
+ Hits 82274 91787 +9513
+ Misses 44911 44800 -111
- Partials 6048 6754 +706
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Jackie-Jiang
force-pushed
the
index_loading_config
branch
from
6002f46
to
e0b7b2d
Compare
Jackie-Jiang
force-pushed
the
index_loading_config
branch
from
e0b7b2d
to
5ab8b0d
Compare
| @@ -870,12 +875,6 @@ public Schema getSchema() { | |||
| return _schema; | |||
| } | |||
|
|
|||
| @VisibleForTesting | |||
| public void setTableConfig(TableConfig tableConfig) { | |||
There was a problem hiding this comment.
👍
I believe logic based on _dirty = true; would be fine, as there are a few other methods setting this flag anyway.
There was a problem hiding this comment.
The main problem is that the setters are not reflected back to table config, which is causing inconsistency across configs
| public IndexLoadingConfig(TableConfig tableConfig, @Nullable Schema schema) { | ||
| extractFromTableConfigAndSchema(tableConfig, schema); | ||
| } | ||
|
|
||
| /** | ||
| * NOTE: Can be used in production code when we want to load a segment as is without any modifications. |
There was a problem hiding this comment.
would this conflict with the case when user indeed wants to 'remove' all indexes?
There was a problem hiding this comment.
When table config and schema are not set, pre-processor is skipped
We should not allow setting
TableConfigintoIndexLoadingConfigafter it is constructed because the step of extracting index settings are not performed.Ideally, we should not allow any setter in
IndexLoadingConfig, and it should always honor table config and schema. That involves bigger change, and will be addressed in followup PRs