Copilot commented on code in PR #4782:
URL: https://github.com/apache/calcite/pull/4782#discussion_r2747472020
##########
file/src/main/java/org/apache/calcite/adapter/file/CsvTableFactory.java:
##########
@@ -48,8 +48,11 @@ public CsvTableFactory() {
final File base =
(File) operand.get(ModelHandler.ExtraOperand.BASE_DIRECTORY.camelName);
final Source source = Sources.file(base, fileName);
+ String separatorString = (String) operand.get("separator");
+ char separator = separatorString != null && separatorString.length() > 0
+ ? separatorString.charAt(0) : ',';
Review Comment:
The separator validation is insufficient. If a user provides an empty string
as the separator, the code will default to a comma due to the length check.
However, this silent fallback could mask configuration errors. Consider adding
validation to reject empty strings explicitly or documenting this behavior
clearly. Additionally, multi-character separators will be silently truncated to
the first character, which could be confusing.
```suggestion
final char separator;
if (separatorString == null) {
separator = ',';
} else if (separatorString.length() == 1) {
separator = separatorString.charAt(0);
} else {
throw new IllegalArgumentException(
"Invalid separator '" + separatorString
+ "'. Separator must be a single character.");
}
```
##########
file/src/main/java/org/apache/calcite/adapter/file/CsvTableFactory.java:
##########
@@ -48,8 +48,11 @@ public CsvTableFactory() {
final File base =
(File) operand.get(ModelHandler.ExtraOperand.BASE_DIRECTORY.camelName);
final Source source = Sources.file(base, fileName);
+ String separatorString = (String) operand.get("separator");
+ char separator = separatorString != null && separatorString.length() > 0
+ ? separatorString.charAt(0) : ',';
Review Comment:
The separator parsing does not validate or handle special characters
properly. Users might want to specify escape sequences like "\t" for tabs or
"\n" for newlines, but the current implementation will treat these as
two-character strings and only take the backslash. Consider adding support for
common escape sequences or documenting that only single literal characters are
supported.
##########
file/src/main/java/org/apache/calcite/adapter/file/CsvTranslatableTable.java:
##########
@@ -25,6 +25,9 @@
import org.apache.calcite.linq4j.Queryable;
import org.apache.calcite.linq4j.tree.Expression;
import org.apache.calcite.plan.RelOptTable;
+
+import au.com.bytecode.opencsv.CSVParser;
+
Review Comment:
The import for au.com.bytecode.opencsv.CSVParser is placed incorrectly
according to the import ordering convention seen in other files in this
package. It should appear after all org.apache.calcite imports but before
org.checkerframework imports. In CsvTable.java (lines 19-28), the correct
ordering is demonstrated: org.apache.calcite imports first, then
au.com.bytecode, then org.checkerframework, then java imports.
##########
file/src/main/java/org/apache/calcite/adapter/file/CsvTableFactory.java:
##########
@@ -48,8 +48,11 @@ public CsvTableFactory() {
final File base =
(File) operand.get(ModelHandler.ExtraOperand.BASE_DIRECTORY.camelName);
final Source source = Sources.file(base, fileName);
+ String separatorString = (String) operand.get("separator");
+ char separator = separatorString != null && separatorString.length() > 0
+ ? separatorString.charAt(0) : ',';
Review Comment:
The new separator feature lacks test coverage. Given that the repository has
comprehensive test coverage for CSV functionality (as seen in
FileAdapterTest.java with tests like testCsvSalesEmps, testCsvSalesDepts,
etc.), this new feature should include tests that verify custom separators work
correctly. Consider adding tests that use different separators like pipe (|) or
tab (\t).
--
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]
