github-actions[bot] commented on PR #61999: URL: https://github.com/apache/doris/pull/61999#issuecomment-4174200096
No issues found in the reviewed change set. Critical checkpoint conclusions: - Goal and correctness: The PR goal is to fix the `create_string_value_set` overload bug and reduce redundant template instantiations in `HybridSet`/`InListPredicateBase`. The code changes do accomplish that goal. The `FunctionIn::open()` call now selects the intended sized overload for string IN predicates, and the storage predicate refactor keeps the string-specialized copy path while collapsing non-string instantiations where `N` is not used. - Scope and minimality: The modification is small and focused on the relevant predicate/set construction paths plus the `FixedContainer::find()` simplification. I did not find unrelated behavior changes. - Concurrency: No new concurrency or lock interaction is introduced in the touched code paths. - Lifecycle management: No special lifecycle or static initialization risks were introduced. Shared ownership of `HybridSetBase` in `InListPredicateBase` remains straightforward. - Configuration: No configuration items were added or changed. - Compatibility: No incompatible storage format, RPC, or function symbol changes were introduced. - Parallel code paths: The two parallel storage predicate creator paths (`IN_LIST` and `NOT_IN_LIST`) were updated consistently. The `FunctionIn` execution path was also updated for the string overload fix. - Conditional checks: The new string-only branch and comments match the actual specialization requirements; I did not find a missing sibling path. - Test coverage: I did not see tests added with this PR. That is the main residual risk: the newly activated fixed-size `StringValueSet` path in `FunctionIn` and the non-string `InListPredicateBase` sharing path would benefit from targeted coverage. - Observability: No new observability appears necessary for this refactor-level change. - Transaction/persistence: Not applicable. - Data writes/modifications: Not applicable. - FE/BE variable passing: Not applicable. - Performance: The direction of the change is sound. The fold-expression rewrite of `FixedContainer::find()` is behaviorally equivalent, and reducing redundant template instantiations should lower compile-time/code-size overhead without changing hot-path semantics. - Other issues: None found from the reviewed paths. Residual risk: - No new regression/unit test coverage was included for the behavior-changing bug fix in `FunctionIn` string-set construction. -- 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]
