peterxcli commented on PR #5194: URL: https://github.com/apache/datafusion-comet/pull/5194#issuecomment-5218283673
@andygrove thanks for another round of review! > 1. “Would you consider removing [the probe-side swap]?” Done. `nested_row_overlap` now always iterates left then right, removing the swap and the `probe_is_left` branch from the inner loop. The corrected benchmarks compare against `b54d9dc40`, which already contains #5176. Four cases improved, although `nested struct long` regressed by about 17%; the PR description reports the complete results. > 2. “Would it read better to move the comparator branch into the `_ =>` arm of the match in `arrays_overlap_list`, so this function stays a true fallback?” Done. Comparator dispatch now occurs directly in the guarded `arrays_overlap_list` match arm. `arrays_overlap_list_generic` handles only otherwise-unhandled types, and the redundant child-type equality check was removed. > 3. “Given #5191 is labeled `correctness` and `priority:high`, could we surface it?” Done for both `arrays_overlap` and `array_position`. Their serdes now report nested floating-point inputs as `Incompatible`, with `getIncompatibleReasons` supplying the explanation for generated compatibility documentation. The main expression table also links to [#5191](https://github.com/apache/datafusion-comet/issues/5191). > 4. “Could you add something like this to `arrays_overlap.sql` so CI picks the fix up when it lands?” Done. The SQL file now covers nested doubles containing `0.0D`, `-0.0D`, and NaN, with `query ignore(https://github.com/apache/datafusion-comet/issues/5191)`. The explicit `D` suffix ensures signed zero is parsed as a double rather than a decimal. > 5. “Would it be worth extending `test_nested_array_sliced_offsets_and_probe_swap` with a row that should come back `null`?” Done. The renamed sliced-offset regression now covers three results within the sliced region: `false`, `null`, and `true`. The null row uses `[[10], NULL]` against `[[20]]`, pinning the absolute-offset null bookkeeping. > 6. “Would you consider adding one nested variant where a match is found partway through?” Done. `nested_int_lists` now accepts an offset, and the new `nested int32 early match` benchmark uses an offset of four. This finds a match partway through each eight-element row instead of always paying the complete `O(n x m)` scan. -- 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]
