gemini-code-assist[bot] commented on code in PR #38272:
URL: https://github.com/apache/beam/pull/38272#discussion_r3131431965
##########
runners/flink/flink_runner.gradle:
##########
@@ -32,9 +32,16 @@ def overrides(versions, type, base_path, group='java') {
["${base_path}/src/${type}/${group}"] + versions.collect {
"${base_path}/${it}/src/${type}/${group}" } + ["./src/${type}/${group}"]
}
-def all_versions = flink_versions.split(",")
-
-def previous_versions = all_versions.findAll { it < flink_major }
+def all_versions = flink_versions.split(",").collect { it.trim() }
+// Determine version order by list position rather than string comparison so
two-digit
+// minors (e.g. "1.20") sort correctly relative to "1.18" / "1.19".
+def flink_major_index = all_versions.indexOf(flink_major)
Review Comment:
To ensure the version lookup is robust against leading/trailing whitespace
in the `flink_major` property (e.g., when passed via command line) and to
handle potential `GString` types, it is recommended to trim and stringify the
value before calling `indexOf`. This maintains consistency with the trimming
performed on `all_versions`.
```
def flink_major_index = all_versions.indexOf(flink_major?.toString()?.trim())
```
##########
runners/flink/flink_runner.gradle:
##########
@@ -32,9 +32,16 @@ def overrides(versions, type, base_path, group='java') {
["${base_path}/src/${type}/${group}"] + versions.collect {
"${base_path}/${it}/src/${type}/${group}" } + ["./src/${type}/${group}"]
}
-def all_versions = flink_versions.split(",")
-
-def previous_versions = all_versions.findAll { it < flink_major }
+def all_versions = flink_versions.split(",").collect { it.trim() }
+// Determine version order by list position rather than string comparison so
two-digit
+// minors (e.g. "1.20") sort correctly relative to "1.18" / "1.19".
+def flink_major_index = all_versions.indexOf(flink_major)
+if (flink_major_index < 0) {
+ throw new GradleException(
+ "flink_major='${flink_major}' is not listed in
flink_versions='${flink_versions}' " +
+ "(see root gradle.properties).")
+}
+def previous_versions = flink_major_index > 0 ? all_versions.subList(0,
flink_major_index) : []
Review Comment:
The ternary operator is redundant here. Since `flink_major_index` is
guaranteed to be non-negative at this point (due to the check on line 39),
`all_versions.subList(0, flink_major_index)` will correctly return an empty
list if `flink_major_index` is 0. Simplifying this makes the code cleaner and
matches the Spark runner implementation mentioned in the PR description.
```
def previous_versions = all_versions.subList(0, flink_major_index)
```
--
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]
