terrymanu edited a comment on issue #2275: Reconstruct rewriting module URL: https://github.com/apache/incubator-shardingsphere/issues/2275#issuecomment-491602194 Some code review suggestion: - [ ] XXXSQLRewriteEngine move to new package name as `engine` - [ ] XXXSQLRewriteEngine need implement a interface name as `SQLRewriteEngine` - [ ] MasterSlaveSQLRewriteEngine use @RequiredArgsConstructor - [ ] Constructor don't need java-doc - [ ] method of XXXSQLRewriteEngine.generateSQL is unnecessary, just call sqlBuilder.toSQL - [ ] Beside Alterable, add Addable interface - [ ] split sharding and encrypt placeholder in package `org.apache.shardingsphere.core.rewrite.placeholder` - [ ] EncryptXXXPlaceholder should not implement `ShardingPlaceholder` - [ ] Don't use @SuppressWarnings("all") on class EncryptUpdateItemColumnPlaceholder - [ ] EncryptUpdateItemColumnPlaceholder has lots of field can be null, consider about the design, maybe split assistedColumn and not has assistedColumn situation - [ ] Question in EncryptWhereColumnPlaceholder, between should not supported - [ ] Should not call SQLUtil.getExactlyValue on rewrite module, current value should come from parse module. For example: IndexPlaceholder - [ ] Rewrite of class InsertSetPlaceholder is out of scope, should not rewrite "SET"
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
