terrymanu commented on PR #39744: URL: https://github.com/apache/shardingsphere/pull/39744#issuecomment-5556624680
### Result **Review Result: Mergeable** The minimal fix has been completed. The strict code review found no SQL semantic, compatibility, performance, or scope issues. ### Evidence - Root cause: Calcite passes a prefix operator’s precedence to its operand during unparsing. Since the existing precedence of `!` was 26, lower than subtraction’s 40, parentheses were not emitted. Raising it to 80—the precedence used by Calcite’s unary `+/-` operators—causes subtraction operands to be parenthesized. See [Calcite 1.42.0 operator definitions](https://github.com/apache/calcite/blob/calcite-1.42.0/core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java#L933-L979), [PREFIX unparsing logic](https://github.com/apache/calcite/blob/calcite-1.42.0/core/src/main/java/org/apache/calcite/sql/SqlSyntax.java#L75-L89), and [parentheses decision logic](https://github.com/apache/calcite/blob/calcite-1.42.0/core/src/main/java/org/apache/calcite/sql/SqlCall.java#L109-L117). - MySQL’s official examples confirm that `! (1+1)` and `! 1+1` have different semantics, so the parentheses must be preserved. See [MySQL 8.4 Logical Operators](https://dev.mysql.com/doc/refman/8.4/en/logical-operators.html). - `SQLExtensionOperatorTable.java:50` → changed only the `NOT_SIGN` precedence from 26 to 80 → preserves parentheses around compound expressions → necessary to fix the root cause. - `NotExpressionConverterTest.java:44` → verifies actual SQL output for compound `!`, simple `!`, and ordinary `NOT` → covers all three regression requirements → necessary to prevent semantic and existing-output regressions. - Before the fix, the regression test failed as expected with the actual output `! order_id - 1000`; after the fix, all three tests passed. - Focused unit test: exit code 0; `Tests run: 3, Failures: 0, Errors: 0`. - Spotless apply/check: exit code 0; both target files are clean. - Checkstyle: exit code 0; zero violations. - The change only updates operator metadata and tests. It introduces no runtime branches, allocations, I/O, or dependencies and is performance-neutral. ### Coverage - Candidate type: standalone local candidate; baseline `bf953abc3057179cbc8d46274edadd4288318b16`. - Both task files were fully reviewed for root cause, blast radius, test validity, SQL dialect semantics, compatibility, performance, style, and convergence. - Ordinary `NOT` retains precedence 26. No other operators, the Calcite version, or Calcite Copy sources were changed. - No unresolved issues were found. As requested, verification was focused; remote CI was not reviewed. - The three pre-existing untracked `__pycache__` directories were excluded and left untouched. - No code was committed or pushed. -- 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]
