lincoln-lil commented on PR #27351: URL: https://github.com/apache/flink/pull/27351#issuecomment-4124055850
## Additional Review Findings Hi @featzhang, thanks for the contribution! I noticed a few additional items that haven't been discussed in previous review rounds: ### 1. [Blocker] Unrelated Triton changes included in this PR This PR includes ~163 lines of changes to the Triton model connector that are completely unrelated to `URL_DECODE_RECURSIVE`: - `docs/layouts/shortcodes/generated/model_triton_advanced_section.html` (+36 lines) - `docs/layouts/shortcodes/generated/triton_configuration.html` (+36 lines) - `flink-models/flink-model-triton/src/main/java/org/apache/flink/model/triton/TritonOptions.java` (+91 lines) These add connection pool management options (`CONNECTION_POOL_MAX_IDLE`, `CONNECTION_POOL_KEEP_ALIVE`, `CONNECTION_POOL_MAX_TOTAL`, `CONNECTION_TIMEOUT`, `CONNECTION_REUSE_ENABLED`, `CONNECTION_POOL_MONITORING_ENABLED`). They should be removed from this PR and submitted separately to keep the PR scope clean and make review/revert/bisect easier. ### 2. [Suggestion] Simplify `MaxDepthArgumentTypeStrategy` using `Number.class` pattern The existing `PercentageArgumentTypeStrategy` handles all numeric types uniformly by using `Number.class` with `getArgumentValue`: ```java Optional<Number> percentageOpt = callContext.getArgumentValue(argumentPos, Number.class); ``` In contrast, `MaxDepthArgumentTypeStrategy` uses a manual switch in `getLiteralAsLong()` to handle `Byte.class`, `Short.class`, `Integer.class`, and `Long.class` individually, plus a `resolveOutputType()` switch. Adopting the `Number.class` approach would: - Eliminate the `getLiteralAsLong()` helper method entirely - Eliminate the `resolveOutputType()` switch - Allow always resolving to `DataTypes.INT()`, which in turn removes 3 of the 4 near-identical `eval` overloads in `UrlDecodeFunction.java` (Byte/Short/Long) ### 3. [Minor] Unrelated modifications to existing `urlDecode()` / `urlEncode()` Javadoc The PR silently modifies the existing Javadoc of `urlDecode()` and `urlEncode()` methods in `BaseExpressions.java` (e.g., "will return null" to "returns null", description rewording). While these are minor improvements, they are not related to the `URL_DECODE_RECURSIVE` feature and ideally should be in a separate cleanup commit or PR. -- 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]
