github-actions[bot] commented on PR #60352: URL: https://github.com/apache/doris/pull/60352#issuecomment-4168206058
No issues found in the reviewed changes. Review summary: - Goal / correctness: The change achieves its stated goal of adding an explode block fast path with guarded fallback, and the added BE tests cover the main batch-truncation, null-array, nullable-element, and fallback scenarios. - Scope / minimality: The modification is reasonably focused to the table-function operator, explode implementations, and supporting tests. - Concurrency: No new concurrency or lock-order concerns were introduced in the reviewed paths. - Lifecycle management: The new fast-path state is reset on open and when a child block is exhausted, which keeps operator-local lifecycle transitions consistent. - Configuration: No new configuration items. - Compatibility: No storage-format, symbol, FE/BE protocol, or rolling-upgrade compatibility issues identified. - Parallel paths consistency: The fast-path capability was wired into both `vexplode` and `vexplode_v2`; unsupported modes still fall back to the existing row-wise path. - Special conditional checks: The contiguous-suffix guard is the key correctness condition for the fast path and is documented in code. - Test coverage: Added BE unit tests cover the new operator-state transitions well, but there is still a residual gap because the fast path is only exercised through mock table functions, not directly through the real `VExplodeTableFunction` / `VExplodeV2TableFunction` implementations, variant inputs, or const-array inputs. - Observability: No additional observability appears necessary for this optimization-only path. - Transaction / persistence / data-write concerns: Not applicable for this change. - FE/BE variable propagation: Not applicable for this change. - Performance: The fast path removes per-row `get_value()` work in the contiguous case without obviously regressing the guarded fallback path. - Other issues: None identified in the reviewed patch. Overall opinion: no actionable findings; residual risk is limited to the missing direct coverage of the real explode implementations on the new fast path. -- 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]
