andygrove commented on PR #5042: URL: https://github.com/apache/datafusion-comet/pull/5042#issuecomment-5876127176
This is a light fully automated review since there are so many PRs open. The `left`/`diag` register caching in `levenshtein_distance` (`native/spark-expr/src/string_funcs/levenshtein.rs:103-119`) is a reasonable way to recover the throughput this PR lost earlier. That same commit (`6858d2e6b`) also removed two tests that existed as of the September 13 revision: `test_batch_scratch_state_leak_order_invariance` and `test_threshold_out_of_band_reset`. Neither is in the test module at `levenshtein.rs:488-582` anymore. The first one was written for the original stale-buffer finding on this PR. It built one batch mixing a very long string, long enough to push the TLS scratch buffers past `MAX_RETAINED_CAPACITY`, together with short strings and small per-row thresholds, then ran that batch through `spark_levenshtein` once in row order and once reversed, and asserted the two results matched. None of the eight tests left in the file drive `spark_levenshtein` with several rows and varying thresholds in a single call, and the existing `levenshtein_threshold.sql` fixture only uses strings a few characters long, so neither would catch scratch state leaking between rows. Since the DP loop changed again in this same commit, could that test, or something equivalent, come back so a future change to the buffer sharing gets caught in CI rather than by hand? -- 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]
