gemini-code-assist[bot] commented on code in PR #36322:
URL: https://github.com/apache/beam/pull/36322#discussion_r2389886346
##########
sdks/java/extensions/sql/jdbc/src/main/java/org/apache/beam/sdk/extensions/sql/jdbc/BeamSqlLine.java:
##########
@@ -53,6 +53,17 @@ private static String[] checkConnectionArgs(String[] args) {
argsList.add(CONNECT_STRING_PREFIX);
}
+ // Add default credentials to prevent interactive prompts
+ if (!argsList.contains("-n")) {
+ argsList.add("-n");
+ argsList.add("beam");
+ }
+
+ if (!argsList.contains("-p")) {
+ argsList.add("-p");
+ argsList.add("beam");
+ }
Review Comment:
While adding default credentials to prevent interactive prompts is a good
idea, the values `"beam"` are hardcoded. For better maintainability and to
avoid magic strings, it would be better to define these as named constants at
the class level, e.g., `private static final String DEFAULT_USER = "beam";` and
`private static final String DEFAULT_PASSWORD = "beam";`.
##########
sdks/java/extensions/sql/jdbc/src/test/java/org/apache/beam/sdk/extensions/sql/jdbc/BeamSqlLineTestingUtils.java:
##########
@@ -49,13 +49,40 @@ public static List<List<String>>
toLines(ByteArrayOutputStream outputStream) {
} catch (UnsupportedEncodingException e) {
throw new RuntimeException(e);
}
- return
outputLines.stream().map(BeamSqlLineTestingUtils::splitFields).collect(toList());
+ return outputLines.stream()
+ .map(BeamSqlLineTestingUtils::parseSqllineOutput)
+ .filter(line -> !line.isEmpty())
+ .collect(toList());
}
- private static List<String> splitFields(String outputLine) {
- return Arrays.stream(outputLine.split("\\|"))
- .map(field -> field.trim())
- .filter(field -> field.length() != 0)
- .collect(toList());
+ private static List<String> parseSqllineOutput(String outputLine) {
+ // Handle sqlline 1.12 table format with borders like +--+, | |, etc.
+ String trimmed = outputLine.trim();
+
+ // Skip table borders and empty lines
+ if (trimmed.isEmpty() || trimmed.matches("^[+\\-|\\s]+$")) {
+ return Arrays.asList();
+ }
+
+ // Parse data rows that contain actual values
+ if (trimmed.startsWith("|") && trimmed.endsWith("|")) {
+ // Remove the outer | characters and split by |
+ String content = trimmed.substring(1, trimmed.length() - 1);
+ return Arrays.stream(content.split("\\|"))
+ .map(field -> field.trim())
+ .filter(field -> !field.isEmpty())
+ .collect(toList());
+ }
+
+ // For non-table format, try the old parsing method
+ if (trimmed.contains("|")) {
+ return Arrays.stream(trimmed.split("\\|"))
+ .map(field -> field.trim())
+ .filter(field -> !field.isEmpty())
+ .collect(toList());
+ }
+
+ // Single value or non-table format (trimmed is not empty at this point)
+ return Arrays.asList(trimmed);
}
Review Comment:
The current implementation of `parseSqllineOutput` has a few issues that
could lead to incorrect parsing of SQL output, especially with NULL values or
trailing empty columns:
1. The `filter(field -> !field.isEmpty())` calls will incorrectly remove
columns that are legitimately empty (representing `NULL`).
2. Using `split("\\|")` without a limit will drop any trailing empty
columns from the result.
3. The logic is a bit complex with multiple return paths for different
formats.
A simplified and more robust implementation could handle all cases more
cleanly by first stripping optional outer pipes, and then splitting while
preserving all fields. This would make the tests more reliable.
```java
private static List<String> parseSqllineOutput(String outputLine) {
// Handle sqlline 1.12 table format with borders like +--+, | |, etc.
String trimmed = outputLine.trim();
// Skip table borders and empty lines
if (trimmed.isEmpty() || trimmed.matches("^[+\\|\\s]+$")) {
return java.util.Collections.emptyList();
}
String content = trimmed;
// For formats enclosed in pipes, remove them before splitting.
if (content.startsWith("|") && content.endsWith("|")) {
content = content.substring(1, content.length() - 1);
}
// Using split with -1 limit preserves trailing empty strings, which is
crucial for NULLs at the end of a row.
return Arrays.stream(content.split("\\|", -1))
.map(String::trim)
.collect(toList());
}
```
--
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]
