Handle null as null value for non-TEXT columns in CSV import #3160
Conversation
Summary of ChangesHello @inv-jishnu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request standardizes the handling of "null" values across different data import formats. Previously, CSV imports treated the string "null" as a literal for non-TEXT columns, causing inconsistencies and potential failures compared to JSON and JSONLines. The changes modify the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly handles string "null" values from CSV imports as null for non-TEXT columns, aligning behavior with other file formats. The implementation is sound, and new unit tests validate the change. My main feedback is to make the "null" string check case-insensitive (e.g., to handle "NULL" and "Null") for better robustness, and to update the corresponding unit tests to cover these cases. This will make the feature more user-friendly.
| if (value != null && !dataType.equals(DataType.TEXT) && value.equals("null")) { | ||
| value = null; | ||
| } |
There was a problem hiding this comment.
The current implementation handles only the lowercase string "null". It would be more robust to handle different cases, such as "NULL" or "Null", as users might provide null values in various formats in CSV files. Using equalsIgnoreCase would make the null handling more flexible and user-friendly.
| if (value != null && !dataType.equals(DataType.TEXT) && value.equals("null")) { | |
| value = null; | |
| } | |
| if (value != null && !dataType.equals(DataType.TEXT) && value.equalsIgnoreCase("null")) { | |
| value = null; | |
| } |
There was a problem hiding this comment.
@ypeckstadt @thongdk8 Should I consider this change as null and NULL is handled differently in JSON and JSONLines file format?
| /** | ||
| * Tests that the string "null" (lowercase) is correctly treated as null for numeric, boolean, and | ||
| * date/time types. | ||
| */ | ||
| @Test | ||
| void createColumnFromValue_valueIsLowercaseNull_shouldReturnNullColumn() | ||
| throws ColumnParsingException { | ||
| String columnName = "testColumn"; | ||
| ColumnInfo columnInfo = ColumnInfo.builder().columnName(columnName).build(); | ||
|
|
||
| // Integer type | ||
| Column<?> intColumn = ColumnUtils.createColumnFromValue(DataType.INT, columnInfo, "null"); | ||
| assertEquals(IntColumn.ofNull(columnName), intColumn); | ||
|
|
||
| // Double type | ||
| Column<?> doubleColumn = ColumnUtils.createColumnFromValue(DataType.DOUBLE, columnInfo, "null"); | ||
| assertEquals(DoubleColumn.ofNull(columnName), doubleColumn); | ||
|
|
||
| // Boolean type | ||
| Column<?> boolColumn = ColumnUtils.createColumnFromValue(DataType.BOOLEAN, columnInfo, "null"); | ||
| assertEquals(BooleanColumn.ofNull(columnName), boolColumn); | ||
|
|
||
| // Date type | ||
| Column<?> dateColumn = ColumnUtils.createColumnFromValue(DataType.DATE, columnInfo, "null"); | ||
| assertEquals(DateColumn.ofNull(columnName), dateColumn); | ||
|
|
||
| // Time type | ||
| Column<?> timeColumn = ColumnUtils.createColumnFromValue(DataType.TIME, columnInfo, "null"); | ||
| assertEquals(TimeColumn.ofNull(columnName), timeColumn); | ||
|
|
||
| // Timestamp type | ||
| Column<?> timestampColumn = | ||
| ColumnUtils.createColumnFromValue(DataType.TIMESTAMP, columnInfo, "null"); | ||
| assertEquals(TimestampColumn.ofNull(columnName), timestampColumn); | ||
| } |
There was a problem hiding this comment.
To accompany the suggested change for case-insensitive null checking in ColumnUtils, this test should also be updated to verify that behavior. This can be done by parameterizing the test to run with different variations of "null" like "NULL" and "Null". This avoids code duplication and makes the test more robust.
You will need to add a MethodSource provider method like the following in the test class:
private static Stream<String> provideNullStrings() {
return Stream.of("null", "NULL", "Null");
} /**
* Tests that the string "null" (case-insensitive) is correctly treated as null for numeric,
* boolean, and date/time types.
*/
@org.junit.jupiter.params.ParameterizedTest
@org.junit.jupiter.params.provider.MethodSource("provideNullStrings")
void createColumnFromValue_valueIsNullString_shouldReturnNullColumn(String nullValue)
throws ColumnParsingException {
String columnName = "testColumn";
ColumnInfo columnInfo = ColumnInfo.builder().columnName(columnName).build();
// Integer type
Column<?> intColumn = ColumnUtils.createColumnFromValue(DataType.INT, columnInfo, nullValue);
assertEquals(IntColumn.ofNull(columnName), intColumn);
// Double type
Column<?> doubleColumn = ColumnUtils.createColumnFromValue(DataType.DOUBLE, columnInfo, nullValue);
assertEquals(DoubleColumn.ofNull(columnName), doubleColumn);
// Boolean type
Column<?> boolColumn = ColumnUtils.createColumnFromValue(DataType.BOOLEAN, columnInfo, nullValue);
assertEquals(BooleanColumn.ofNull(columnName), boolColumn);
// Date type
Column<?> dateColumn = ColumnUtils.createColumnFromValue(DataType.DATE, columnInfo, nullValue);
assertEquals(DateColumn.ofNull(columnName), dateColumn);
// Time type
Column<?> timeColumn = ColumnUtils.createColumnFromValue(DataType.TIME, columnInfo, nullValue);
assertEquals(TimeColumn.ofNull(columnName), timeColumn);
// Timestamp type
Column<?> timestampColumn =
ColumnUtils.createColumnFromValue(DataType.TIMESTAMP, columnInfo, nullValue);
assertEquals(TimestampColumn.ofNull(columnName), timestampColumn);
}1 participant
Description
This PR updates the column creation logic in createColumnFromValue() in ColumnUtils class to correctly interpret the string "null" as a null value for non-TEXT column types.
Previously, when CSV files contained null as a value it was parsed as "null" which caused the initial null check (value != null) and which caused a different behaviour when same value (null) is passed via JSON or JSONLines file type
This caused CSV-based imports to fail even though JSON and JSON Lines imports handled null values correctly.
The fix ensures consistent behavior across all file types by converting null to actual null values for all column types except TEXT when provided via a CSV file.
Related issues and/or PRs
NA
Changes made
Updated createColumnFromValue() method in ColumnUtils class and added a pre-check:
Added Unit Tests in ColumnUtilsTest
Checklist
Additional notes (optional)
NA
Release notes
Handle null as null value for non-TEXT columns in CSV import