terrymanu commented on issue #36454: URL: https://github.com/apache/shardingsphere/issues/36454#issuecomment-3592445436
Thank you very much for the contribution and for providing the detailed design explanation in the issue. The optimization goal (sharding-aware IN predicate rewrite) is valuable, and the high-level design direction is reasonable. However, after reviewing the current PR, the overall change set is still too large and crosses several core modules: 1. The modification scope is very wide This PR touches multiple layers at the same time, including: Sharding algorithm SPI Sharding rewrite token system RouteSQLRewriteEngine in infra rewrite core New interface (ParameterFilterable) SQL rewrite pipeline integration Many new types + large generator logic IT tests + binder interactions Any issue in these areas can introduce subtle problems such as wrong routing, parameter index mismatches, inconsistent rewrite results, or silent data deviation. 2. The PR introduces structural changes that need architectural discussion The approach of filtering parameters per-route using a new interface and extending the rewrite engine is conceptually correct, but it changes the core rewrite flow. Such changes require agreement on: How parameter filtering should be modeled Whether this should be a generic rewrite capability or sharding-specific How to guarantee compatibility with existing token generators How to avoid breaking custom sharding algorithms How to handle complex SQL (subqueries, nested IN, multi-sharding-keys) These points still need further design refinement before implementation can safely move forward. 3. The change is too large to review and validate safely in one PR This PR includes 1300+ lines across 10+ files. Given the sensitivity of the modules involved, it is not suitable to merge such a large change at once. We risk introducing regressions that are difficult to detect through unit tests alone. 4. Suggested next steps To proceed safely, we recommend: Continue design discussion in the issue (e.g., #36454) Clarify module boundaries, SPI behavior, rewrite engine responsibilities, and parameter-filter design. Split the implementation into smaller, incremental PRs, for example: Introduce the new rewrite extension point in isolation. Add minimal sharding IN optimization for simple cases. Gradually extend to complex sharding, placeholders, subqueries, etc. Add more targeted tests per step so review becomes manageable and regressions easier to detect. Conclusion The problem and the proposed direction are reasonable, but the current PR is too large and involves core architecture changes. We need further design iteration and smaller, incremental PRs before it can be safely accepted. Thank you again for the effort—happy to continue the discussion and help refine the approach. -- 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]
