R0B2133 commented on PR #23188: URL: https://github.com/apache/datafusion/pull/23188#issuecomment-5169451101
Hi @oc7o — thanks for driving this fix for #22263; the translator and test coverage read well. Reviewing at head `c66c49247174e20f5c13ea44ebaf7af1f5977dd5`, I found an additional PostgreSQL-compatibility question. When `ESCAPE` is omitted, PostgreSQL treats `\` as the default escape character for `SIMILAR TO`. PostgreSQL documents that default as nonstandard: under the SQL standard, omitting `ESCAPE` means there is no escape character ([PostgreSQL docs](https://www.postgresql.org/docs/current/functions-matching.html#FUNCTIONS-SIMILARTOREGEXP)). This is separate from whether the PR satisfies #22263's explicit objective. At this head, the translator escapes `\` to `\\` unconditionally, so these results differ from PostgreSQL: | Expression | PostgreSQL | This PR | |---|---:|---:| | `'a%c' SIMILAR TO 'a\%c'` | `true` | `false` | | `'a_c' SIMILAR TO 'a\_c'` | `true` | `false` | | `'a\c' SIMILAR TO 'a\\c'` | `true` | `false` | | `'a\\c' SIMILAR TO 'a\\c'` | `false` | `true` | A column-supplied, non-literal pattern showed the same difference. In the focused `strings.slt` run, the four cases above plus the column-dependent case were the only five new failures; three new negative controls and every pre-existing case passed. This was only a focused run, so I am not making a full-suite or broader regression claim. Would you and the maintainers prefer to cover PostgreSQL's default escape in this PR, or in a follow-up? I can share the exact sqllogictest block with positive, opposite-direction, column-dependent, and negative-control cases. One remaining design question is how sequences PostgreSQL passes through to its regex engine, such as `\d` and `\m`, should behave if `\` becomes an escape character. -- 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]
