Phixsura commented on PR #38704: URL: https://github.com/apache/shardingsphere/pull/38704#issuecomment-4527926709
> 决定 > > 合并结论：不可合并 > > 审查范围：PR #38704最新 head [e8bfdc6](https://github.com/apache/shardingsphere/commit/e8bfdc6fb0822b6ca856cecc859ac5e9a55642e2)；kernel/sql-federation/compiler 中的 8 个文件；关联问题#31897、#31898、#31899；MySQL 字符串函数语义；MySQL/PostgreSQL/openGauss/Oracle/default 的共享 SQL Federation 运算符表路径。 > > 未审查范围：没有完整的本地 Maven 运行或实时 MySQL/PostgreSQL 运行时执行；依赖于最新的 GitHub 检查以及静态/根本原因审查。 > > 需要专家审核：推荐一位 SQL Federation/Calcite 操作符表审核员。 > > 积极反馈 > > 该 PR 通过添加专用运算符条目和针对简单 LENGTH、BIT_LENGTH 和 LOCATE 情况的单元覆盖率，朝着正确的方向发展。除了跳过矩阵阶段外，此更改是局部性的，并且在最新版本中已获得 CI 认证。 > > 主要问题 > > [P1] #31899尚未完全修复。报告的 LOCATE(x'64', _utf8mb4'abcdef') 失败是由于不支持的 CommonExpressionSegment 引起的，但 ExpressionConverter.java（第 97 行）仍然会抛出 CommonExpressionSegment 异常。该 PR 仅添加了一个元数据接受 STRING 操作数的 LOCATE 运算符，并且新的编译器用例仅涵盖纯字符字面量。请修复二进制/十六进制字面量转换并添加确切的回归用例，或者暂时不要关闭#31899的该部分。 > > [P1] MySQL 字符串语义对字符集/排序规则的支持尚不完善。MySQLLengthFunction.java（第 61 行）和 MySQLBitLengthFunction 硬编码了 UTF-8 字节计数。MySQL 文档中将 LENGTH() 函数定义为字符串的字节长度，并支持诸如 _latin1 和 _binary 之类的字符集引入符；LOCATE() 函数仅在参数为二进制时区分大小写。这导致了一些反例，例如 _latin1'Müller' 的字节长度以及非二进制参数的不区分大小写的 LOCATE 函数。请使实现能够感知字符集/排序规则，或者通过测试和审核人员的同意来明确缩小支持的行为范围。参考资料：[MySQL 字符串函数](https://dev.mysql.com/doc/refman/8.4/en/string-functions.html)，[MySQL 字符集引入符](https://dev.mysql.com/doc/refman/8.4/en/charset-introducer.html)。 > > [P1] 非 MySQL SQL 联盟的爆炸半径风险仍未解决。CompilerContextFactory.java（第 77 行）始终在数据库特定的 Calcite 库之前链接 new MySQLOperatorTable()，因此添加 LENGTH、BIT_LENGTH 和 LOCATE 会将 MySQL 的行为暴露给 PostgreSQL、openGauss、Oracle 和回退/默认上下文。PostgreSQL 文档中描述了 bit_length、octet_length、position 和 strpos，但没有描述 MySQL 的 LOCATE(substr, str, pos) 语义。请将这些函数限制为仅对 MySQL 可用，或者添加明确的非目标方言反例来证明不存在语义回归。来源：[PostgreSQL 字符串函数](https://www.postgresql.org/docs/current/functions-string.html)。 > > 新引入的问题 > > `git diff --check` 命令报告新文件中存在尾随空格，并且新的参数化测试使用了类似 `length({0}) -> {1}` 的显示名称，而不是必需的 `{0}` 模板。请在下次修订之前清理这些问题。 > > 后续步骤 > > 针对具体问题 SQL 添加生产路径回归测试，包括 _latin1、_utf8mb4、二进制/十六进制字面量、列/WHERE 用法和嵌套函数情况。 > > 至少对 PostgreSQL/openGauss 添加跨方言验证，以证明 MySQL 专用函数不会泄漏，或者将 MySQLOperatorTable 注册限制为 MySQL。 > > 重新运行带有依赖项的作用域验证，例如 ./mvnw -pl kernel/sql-federation/compiler -am -DskipITs -Dspotless.skip=true -Dtest=SQLStatementCompilerIT,MySQLLengthFunctionTest,MySQLBitLengthFunctionTest,MySQLLocateFunctionTest test。 > 决定 > > 合并结论：不可合并 > > 审查范围：PR #38704最新 head [e8bfdc6](https://github.com/apache/shardingsphere/commit/e8bfdc6fb0822b6ca856cecc859ac5e9a55642e2)；kernel/sql-federation/compiler 中的 8 个文件；关联问题#31897、#31898、#31899；MySQL 字符串函数语义；MySQL/PostgreSQL/openGauss/Oracle/default 的共享 SQL Federation 运算符表路径。 > > 未审查范围：没有完整的本地 Maven 运行或实时 MySQL/PostgreSQL 运行时执行；依赖于最新的 GitHub 检查以及静态/根本原因审查。 > > 需要专家审核：推荐一位 SQL Federation/Calcite 操作符表审核员。 > > 积极反馈 > > 该 PR 通过添加专用运算符条目和针对简单 LENGTH、BIT_LENGTH 和 LOCATE 情况的单元覆盖率，朝着正确的方向发展。除了跳过矩阵阶段外，此更改是局部性的，并且在最新版本中已获得 CI 认证。 > > 主要问题 > > [P1] #31899尚未完全修复。报告的 LOCATE(x'64', _utf8mb4'abcdef') 失败是由于不支持的 CommonExpressionSegment 引起的，但 ExpressionConverter.java（第 97 行）仍然会抛出 CommonExpressionSegment 异常。该 PR 仅添加了一个元数据接受 STRING 操作数的 LOCATE 运算符，并且新的编译器用例仅涵盖纯字符字面量。请修复二进制/十六进制字面量转换并添加确切的回归用例，或者暂时不要关闭#31899的该部分。 > > [P1] MySQL 字符串语义对字符集/排序规则的支持尚不完善。MySQLLengthFunction.java（第 61 行）和 MySQLBitLengthFunction 硬编码了 UTF-8 字节计数。MySQL 文档中将 LENGTH() 函数定义为字符串的字节长度，并支持诸如 _latin1 和 _binary 之类的字符集引入符；LOCATE() 函数仅在参数为二进制时区分大小写。这导致了一些反例，例如 _latin1'Müller' 的字节长度以及非二进制参数的不区分大小写的 LOCATE 函数。请使实现能够感知字符集/排序规则，或者通过测试和审核人员的同意来明确缩小支持的行为范围。参考资料：[MySQL 字符串函数](https://dev.mysql.com/doc/refman/8.4/en/string-functions.html)，[MySQL 字符集引入符](https://dev.mysql.com/doc/refman/8.4/en/charset-introducer.html)。 > > [P1] 非 MySQL SQL 联盟的爆炸半径风险仍未解决。CompilerContextFactory.java（第 77 行）始终在数据库特定的 Calcite 库之前链接 new MySQLOperatorTable()，因此添加 LENGTH、BIT_LENGTH 和 LOCATE 会将 MySQL 的行为暴露给 PostgreSQL、openGauss、Oracle 和回退/默认上下文。PostgreSQL 文档中描述了 bit_length、octet_length、position 和 strpos，但没有描述 MySQL 的 LOCATE(substr, str, pos) 语义。请将这些函数限制为仅对 MySQL 可用，或者添加明确的非目标方言反例来证明不存在语义回归。来源：[PostgreSQL 字符串函数](https://www.postgresql.org/docs/current/functions-string.html)。 > > 新引入的问题 > > `git diff --check` 命令报告新文件中存在尾随空格，并且新的参数化测试使用了类似 `length({0}) -> {1}` 的显示名称，而不是必需的 `{0}` 模板。请在下次修订之前清理这些问题。 > > 后续步骤 > > 针对具体问题 SQL 添加生产路径回归测试，包括 _latin1、_utf8mb4、二进制/十六进制字面量、列/WHERE 用法和嵌套函数情况。 > > 至少对 PostgreSQL/openGauss 添加跨方言验证，以证明 MySQL 专用函数不会泄漏，或者将 MySQLOperatorTable 注册限制为 MySQL。 > > 重新运行带有依赖项的作用域验证，例如 ./mvnw -pl kernel/sql-federation/compiler -am -DskipITs -Dspotless.skip=true -Dtest=SQLStatementCompilerIT,MySQLLengthFunctionTest,MySQLBitLengthFunctionTest,MySQLLocateFunctionTest test。 Thanks for the thorough review, all three P1s hold up. On P1.1: I misread #31899 as a single task. It's actually a checkbox list and the `LOCATE(x'64', _utf8mb4'abcdef')` case has its real root cause in `ExpressionConverter.java:97` (CommonExpressionSegment), which this PR doesn't touch. I'll drop `Closes #31899` from the description and leave that case for a separate fix. On P1.2: Acknowledged on charset/collation. Would you prefer (a) narrowing the implementation to ASCII-only with explicit tests and Javadoc, or (b) making it charset-aware? I haven't traced how Calcite surfaces charset/collation info into UDFs yet, so (b) is open-ended for me. On P1.3: I'd like to confirm scope here. `CompilerContextFactory.getOperatorTables()` already prepends `new MySQLOperatorTable()` for every dialect today, but this PR is what makes the leak actually break things, since LENGTH is the first registered function with a real semantic clash against PostgreSQL/openGauss/Oracle. Two options: 1. Restrict registration to the MySQL context inside this PR (touches `CompilerContextFactory`, expands the diff). 2. Drop LENGTH/BIT_LENGTH/LOCATE from this PR entirely and submit an architecture-fix PR first. Which would you prefer? On the nits: trailing whitespace and the parameterized-test display names will be cleaned up in the next revision. -- 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]
