You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
The arrayToCsv method does not handle CSV special characters. If an element contains the delimiter, a double-quote, or a newline, the output will be malformed CSV. Proper CSV encoding should quote fields containing special characters and escape internal double-quotes (e.g., RFC 4180 compliance).
publicstaticStringarrayToCsv(List<Object> array, Stringdelimiter) {
if (array == null) {
returnnull;
}
if (delimiter == null) {
delimiter = ",";
}
if (array.isEmpty()) {
return"";
}
StringBuilderresult = newStringBuilder();
for (inti = 0; i < array.size(); i++) {
if (i > 0) {
result.append(delimiter);
}
Objectelement = array.get(i);
if (element != null) {
result.append(element.toString());
}
}
returnresult.toString();
}
The constructor uses NullPolicy.ARG0, which means the function returns null when the first argument (the array) is null — this is handled correctly. However, the arrayToCsv static method also has an explicit null check for the array. These are redundant but consistent. More importantly, if NullPolicy.ARG0 short-circuits before calling arrayToCsv, the null-delimiter fallback logic inside arrayToCsv will still be reached when the array is non-null but the delimiter is null. This is fine, but the interaction between the null policy and the runtime method should be validated to ensure no unexpected behavior when the delimiter argument is null at the Calcite expression level.
Other array functions like ARRAY_SLICE and ARRAY_COMPACT are registered with true (indicating they are variadic or have special handling) in BuiltinFunctionName. It should be verified whether ARRAY_TO_CSV also needs such a flag, given it supports an optional second argument.
Latest suggestions up to baa3553
Explore these optional code suggestions:
Category
Suggestion
Impact
General
Use explicit null policy for consistent behavior
Using NullPolicy.ARG0 means the function will return null if the first argument (the array) is null, which is the intended behavior. However, when a two-argument form is used and the second argument (delimiter) is null, the NullPolicy.ARG0 policy will NOT short-circuit on a null delimiter — the arrayToCsv method handles this case by defaulting to ",". This is correct, but it should be verified that NullPolicy.ARG0 does not also short-circuit on ARG1 being null in the two-argument case, potentially bypassing the null-delimiter fallback logic. Consider using NullPolicy.NONE and handling all null checks explicitly in arrayToCsv to ensure consistent behavior.
Why: The suggestion raises a valid concern about NullPolicy.ARG0 potentially short-circuiting on the first argument while the second argument's null handling is done manually in arrayToCsv. Switching to NullPolicy.NONE ensures all null checks are handled explicitly in the method, providing more predictable behavior. This is a moderate correctness/consistency concern.
Low
Clarify null element handling in loop
The current implementation skips null elements entirely (no output for null), but the tests expect null elements to produce empty strings between delimiters (e.g., "GET,,WRITE"). The if (element != null) block should use an else branch to append an empty string, or simply call result.append(element != null ? element.toString() : ""). The current code actually does produce the correct output since the delimiter is appended regardless, but the logic is misleading — the if (element != null) block effectively appends nothing for null, which is correct. However, to make the intent explicit and avoid future bugs, use a ternary expression.
public static String arrayToCsv(List<Object> array, String delimiter) {
if (array == null) {
return null;
}
if (delimiter == null) {
delimiter = ",";
}
if (array.isEmpty()) {
return "";
}
StringBuilder result = new StringBuilder();
for (int i = 0; i < array.size(); i++) {
if (i > 0) {
result.append(delimiter);
}
Object element = array.get(i);
- if (element != null) {- result.append(element.toString());- }+ result.append(element != null ? element.toString() : "");
}
return result.toString();
}
Suggestion importance[1-10]: 4
__
Why: The suggestion correctly identifies that the current code produces the right output but the intent is unclear. Replacing the if (element != null) block with a ternary expression makes the null-handling intent explicit and reduces the chance of future bugs. This is a minor readability/maintainability improvement.
Low
Previous suggestions
Suggestions up to commit 1a85890
Category
Suggestion
Impact
Possible issue
Fix null policy to cover all arguments
The class uses NullPolicy.ARG0, which means the implementor is only called when the first argument (the array) is non-null. However, the second argument (delimiter) could still be null at runtime, and the arrayToCsv method handles that by defaulting to ",". This is fine, but the NotNullImplementor contract assumes non-null inputs — if the delimiter expression evaluates to null at the Calcite expression level, it may not be passed correctly. Consider using NullPolicy.ANY or NullPolicy.NONE to ensure consistent null handling for both arguments.
-public static class ArrayToCsvImplementor implements NotNullImplementor {- @Override- public Expression implement(- RexToLixTranslator translator, RexCall call, List<Expression> translatedOperands) {- // Handle both 1-argument (with default delimiter) and 2-argument cases- if (translatedOperands.size() == 1) {- ...- } else if (translatedOperands.size() == 2) {- ...- } else {- throw new IllegalArgumentException(- "ARRAY_TO_CSV expects 1 or 2 arguments, got " + translatedOperands.size());- }- }+public ArrayToCsvFunctionImpl() {+ super(new ArrayToCsvImplementor(), NullPolicy.ANY);
}
Suggestion importance[1-10]: 5
__
Why: Using NullPolicy.ARG0 means only the first argument's nullability is checked, but the second argument (delimiter) could be null at the Calcite expression level. Changing to NullPolicy.ANY would ensure consistent null handling for both arguments, though the arrayToCsv method already handles null delimiters gracefully.
Low
General
Use safer null-tolerant string conversion
When an element is null, the delimiter is still appended but no value is added, which produces correct output (e.g., "a,,b"). However, the leading-null case (e.g., [null, "a"]) produces ",a" — a leading delimiter — which may be unexpected. The current behavior is tested and accepted, but consider using String.join with null-to-empty-string mapping via streams for clarity and correctness consistency. More critically, using String.valueOf(element) instead of element.toString() avoids a potential NullPointerException if the null check is ever removed or refactored.
for (int i = 0; i < array.size(); i++) {
if (i > 0) {
result.append(delimiter);
}
Object element = array.get(i);
if (element != null) {
- result.append(element.toString());+ result.append(String.valueOf(element));
}
}
Suggestion importance[1-10]: 2
__
Why: The change from element.toString() to String.valueOf(element) is functionally equivalent here since there's already a null check (if (element != null)) guarding the call. The suggestion provides minimal safety improvement since the null check already prevents NPE.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ishag4
requested review from
LantaoJin,
RyanL1997,
Swiddis,
acarbonetto,
ahkcs,
anirudha,
dai-chen,
joshuali925,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
songkant-aws,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
Description
[Describe what this change achieves]
Related Issues
Resolves #4587
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.