sameerpandey17 commented on PR #75635: URL: https://github.com/apache/airflow/pull/75635#issuecomment-6467068651
> Here are my thoughts: > > 1. Your benchmark shows a clear performance regression (roughly 4–5× slower for selective prefixes) but given that this fixes a legitimate correctness bug, I think the trade-off is justified. > 2. Could the remaining lower bound cause a similar correctness issue? For example, if a collation placed `ch` before `c`, searching for prefix `c` would exclude `chata` because it fails `column >= 'c'`, despite matching the prefix. Is there a guarantee that the lower bound is safe across supported collations? @SameerMesiah97 agreed that fixing the correctness bug comes first, and thanks for the second question. I tested it instead of guessing. There is no guarantee the lower bound is safe. I checked every ICU collation on Postgres 16.15: 871 collations across 241 languages, with ASCII IDs (letters, digits and `_ - . : +`, prefixes of 1–2 characters, 171,338 prefix/string pairs), testing the original upper bound (`< bumped`) and the lower bound (`>= prefix`). | approach | collations correct | languages with no failing collation | |---|---|---| | original (both bounds) | 787 / 871 (90.4%) | 210 / 241 (87.1%) | | this PR (lower bound only) | 867 / 871 (99.5%) | 239 / 241 (99.2%) | | no bounds (`substr` only) | 871 / 871, correct by construction (not part of the audit) | 241 / 241 | Of the 84 collations (9.6%) where the original code was wrong, this PR fixes 80 (95.2%) and leaves 4 (4.8%), which fail exactly as you described: Welsh (`cy`, `cy-GB`: prefix `n` misses `ng…`) and Igbo (`ig`, `ig-NG`: prefix `c` misses `ch…`). The database default `en_US.utf8` has no failures with either bound. The original code has the same lower bound, so this PR didn't introduce those 4, but it doesn't fix them either. Dropping `col >= lower` as well would fix those 4 and make the filter correct everywhere. The cost is speed: @yuseok89 measured up to 10.6 s on skewed data after removing the upper bound alone, and removing the lower bound too can only add to that. So I see three options and would like your view: 1. Keep this PR as it is (fixes 80 of the 84 broken collations) and track Welsh/Igbo separately. 2. Also drop the lower bound here for full correctness, accepting the slowdown. 3. Keep the range where the collation is safe and fall back to `substr`-only only where it isn't, or an index-level fix, as a separate change. Which would you prefer for this PR? Caveats: ICU collations only (glibc locales like `cs_CZ.utf8` not covered), ASCII IDs and short prefixes, so a strong check but not a proof. The percentages count collations, not deployments, and most installs use `en_US` or `C`. -- 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]
