Cleanup and converge V1 and V2 SchemaConformingTransformer #14772
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14772 +/- ##
============================================
+ Coverage 61.75% 63.83% +2.08%
- Complexity 207 1610 +1403
============================================
Files 2436 2702 +266
Lines 133233 150807 +17574
Branches 20636 23283 +2647
============================================
+ Hits 82274 96272 +13998
- Misses 44911 47316 +2405
- Partials 6048 7219 +1171
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Jackie-Jiang
added
backward-incompat
| private void setSchemaConformingTransformerV2Config(JsonNode schemaConformingTransformerV2Config) { | ||
| // Map _schemaConformingTransformerV2Config JSON into SchemaConformingTransformerConfig | ||
| if (schemaConformingTransformerV2Config != null && _schemaConformingTransformerConfig == null) { | ||
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| _schemaConformingTransformerConfig = | ||
| objectMapper.convertValue(schemaConformingTransformerV2Config, SchemaConformingTransformerConfig.class); | ||
| } | ||
| } |
There was a problem hiding this comment.
Does it work the same if we simply put:
| private void setSchemaConformingTransformerV2Config(JsonNode schemaConformingTransformerV2Config) { | |
| // Map _schemaConformingTransformerV2Config JSON into SchemaConformingTransformerConfig | |
| if (schemaConformingTransformerV2Config != null && _schemaConformingTransformerConfig == null) { | |
| ObjectMapper objectMapper = new ObjectMapper(); | |
| _schemaConformingTransformerConfig = | |
| objectMapper.convertValue(schemaConformingTransformerV2Config, SchemaConformingTransformerConfig.class); | |
| } | |
| } | |
| public void setSchemaConformingTransformerV2Config(SchemaConformingTransformerConfig schemaConformingTransformerConfig) { | |
| _schemaConformingTransformerConfig = schemaConformingTransformerConfig; | |
| } |
All we need to do is mapping both key to the same property right?
There was a problem hiding this comment.
All we need to do is mapping both key to the same property right?
Yes, but would the suggested change work in the same way?
In which case, the setSchemaConformingTransformerV2Config(SchemaConformingTransformerConfig schemaConformingTransformerConfig) would be called? I thought when
{"schemaConformingTransformerV2Config" : {...}}
appears in Json, it would internally trigger setSchemaConformingTransformerV2Config(JsonNode schemaConformingTransformerV2Config) only (JsonNode as type).
I need eventually the above json could create the schemaConformingTransformerConfig object.
There was a problem hiding this comment.
Based on my understanding, it should be able to directly construct the SchemaConformingTransformerConfig properly. You can probably add a test for backward compatibility
There was a problem hiding this comment.
Good idea! Added the UT. Verified that this works. Thanks for the proposal.
Jackie-Jiang
added
cleanup
backward-incompat
pinot-spi/src/test/java/org/apache/pinot/spi/config/table/IndexingConfigTest.java
Outdated
Show resolved
Hide resolved
refactorbackward-incompatFollowup on #14546
Next step:
Update https://docs.pinot.apache.org/basics/data-import/pinot-input-formats/schema-conforming-transformer page with a complete story on how to ingest/store/index large scale dynamic schema data like logging