github-actions[bot] commented on PR #61856: URL: https://github.com/apache/doris/pull/61856#issuecomment-4154478333
## Code Review Summary ### PR Goal Remove the `deep_copy` virtual method from `TypeInfo` and all its subclasses (`ScalarTypeInfo`, `ArrayTypeInfo`, `MapTypeInfo`, `StructTypeInfo`), along with the corresponding `FieldTypeTraits::deep_copy` static methods. The two callers are replaced with direct/inlined alternatives. ### Critical Checkpoint Conclusions **1. Does the code accomplish the goal? Is there a test that proves it?** Yes. The `deep_copy` virtual method is fully removed from `TypeInfo` and all subclasses. No callers remain in `be/src/` or `be/test/`. Tests are updated to remove `deep_copy` test coverage (which is correct since the function no longer exists). The test is renamed from `copy_and_equal` to `cmp_and_minmax` to accurately reflect remaining coverage. **2. Is this modification as small, clear, and focused as possible?** Yes. The PR touches only the files necessary: the type hierarchy (`types.h`), the two callers (`bloom_filter_index_writer`, `indexed_column_writer`), and the test file. **3. Concurrency concerns?** Not applicable — no concurrency changes. **4. Lifecycle management?** Not applicable — no lifecycle changes. **5. Configuration items added?** None. **6. Incompatible changes (function symbols, storage formats)?** No. `deep_copy` was an internal virtual method, not part of any serialization or storage format. No compatibility concerns. **7. Functionally parallel code paths?** The `PrimaryKeyBloomFilterIndexWriterImpl` had its `_type_info` removed (correct). The template `BloomFilterIndexWriterImpl` (anonymous namespace, same .cpp file) has pre-existing dead `_type_info` and `_arena` members that were never used even before this PR. While cleaning them up here would be ideal for consistency, this is pre-existing dead code and not a blocking issue. **8. Special conditional checks?** The new inline Slice copy in `PrimaryKeyBloomFilterIndexWriterImpl::add_values` checks `v->size > 0` before allocating, setting `data = nullptr` for empty slices. This is correct and matches the behavior of the removed `FieldTypeTraits<CHAR>::deep_copy`. **9. Test coverage?** The removed tests tested `deep_copy` specifically, which no longer exists. Remaining tests for `cmp`, `set_to_min`, `set_to_max` are preserved. The `ArrayTypeTest` deep_copy test is also correctly removed. Adequate. **10. Observability?** Not applicable — no observability changes needed for a dead-code removal. **11. Transaction/persistence?** Not applicable. **12. Data writes and modifications?** Not applicable — no data path changes beyond the two callers which are functionally equivalent (verified). **13. FE-BE variable passing?** Not applicable. **14. Performance analysis?** The `IndexedColumnWriter` change is a minor **performance improvement**: the old code deep-copied the first value into a buffer, then encoded from that buffer. The new code encodes directly from the source value, eliminating one redundant copy and the `_arena` + `_first_value` (faststring) allocations. The `PrimaryKeyBloomFilterIndexWriterImpl` change is neutral — it replaces one copy mechanism with an equivalent inline one. **15. Other issues?** - **Minor (pre-existing, non-blocking):** `BloomFilterIndexWriterImpl` (template class in anonymous namespace, `bloom_filter_index_writer.cpp:64-174`) still has unused `_type_info` and `_arena` members. The `_type_info` is stored but never read. The `_arena` is never allocated from, so `_arena.used_size()` in `size()` always returns 0. Consider cleaning these up in a follow-up or in this PR for completeness. ### Verdict **No blocking issues found.** The refactoring is correct, complete (no remaining callers), and the replacement implementations are semantically equivalent to the originals. The pre-existing dead members in `BloomFilterIndexWriterImpl` are a minor cleanup opportunity. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
