HappenLee commented on PR #69296: URL: https://github.com/apache/doris/pull/69296#issuecomment-6340891573
Reviewed the latest head, `81992a12f42db6ad01cdebd7ab5edbcd2161ebdf`. I have one confirmed test defect and two performance/validation points to address before merging: 1. **[P2] The typed lookup test's expected values do not match its actual WHEN keys.** I independently confirmed the [existing review finding](https://github.com/apache/doris/pull/69296#discussion_r4346233050), which still applies to this head. With `TYPE_INT` and 63 branches, the oracle assumes keys `0..62`, but CASE construction replaces the last key with duplicate `1`. Non-NULL row 62 therefore expects `63`, although the correct CASE result is ELSE `-1`. The mismatch also occurs at larger branch counts. Please derive expected results from the actual inserted key sequence, preserving first-match semantics, NULLs and misses. This currently makes correct linear and lookup implementations fail the new test. 2. **The numeric result merge still scans the batch once per evaluated result column.** `_execute_simple_case_lookup()` reduces key matching to one hash lookup per non-NULL row, but `update_result_auto_simd()` subsequently loops over each populated THEN/ELSE column and all rows. For non-nullable numeric results, merging remains approximately `O(rows * evaluated_result_columns)`; batches hitting many branches can therefore remain quadratic in rows/branches overall. Please benchmark the final implementation with concentrated and distributed hits, misses, constant and dynamic THEN expressions, and branch counts around the lookup thresholds. Measuring a per-row gather against the existing SIMD merge would help establish whether result assembly becomes the next bottleneck. The reported early lookup-versus-linear speedups do not establish the final implementation's end-to-end benefit. 3. **Please validate the final source version, including the default searched CASE path.** The PR description states that the final type, duplicate-key and allocator changes have not been rebuilt or retested; the latest output-compaction commit also says it ran against the older `448cb839` service. Please rebuild and run the corrected BE tests, relevant FE tests and regressions against the final implementation. Cover both settings of `enable_experimental_simple_case`: the constant-THEN broadcasting change in the shared result-merging helper also affects searched CASE when the experimental setting is disabled. My verification here was static code tracing and independent fixture calculations; I did not build or execute the final Doris implementation. -- 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]
