-
Notifications
You must be signed in to change notification settings - Fork 614
Added getShortArray & getStringArray implementation #2604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...rc/main/java/com/clickhouse/client/api/data_formats/internal/AbstractBinaryFormatReader.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // write data | ||
| BinaryStreamUtils.writeVarInt(out, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like we have array of data and doing it in the loop - it keep test code compact.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other comments (1)
- client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/AbstractBinaryFormatReader.java (648-648) The error messages in `getStringArray` are inconsistent with other array getter methods. For consistency, consider using similar error messages as the other array methods.
💡 To request another review, post a new comment with "/windsurf-review".
| public String[] getStringArray(String colName) { | ||
| Object value = readValue(colName); | ||
| if (value instanceof BinaryStreamReader.ArrayValue) { | ||
| BinaryStreamReader.ArrayValue array = (BinaryStreamReader.ArrayValue) value; | ||
| int length = array.length; | ||
| if (!array.itemType.equals(String.class)) | ||
| throw new ClientException("Not A String type."); | ||
| String [] values = new String[length]; | ||
| for (int i = 0; i < length; i++) { | ||
| values[i] = (String)((BinaryStreamReader.ArrayValue) value).get(i); | ||
| } | ||
| return values; | ||
| } | ||
| throw new ClientException("Not ArrayValue type."); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getStringArray implementation should follow the same pattern as other array getters by using the getPrimitiveArray method. This would make the code more consistent and reduce duplication.
| public String[] getStringArray(String colName) { | |
| Object value = readValue(colName); | |
| if (value instanceof BinaryStreamReader.ArrayValue) { | |
| BinaryStreamReader.ArrayValue array = (BinaryStreamReader.ArrayValue) value; | |
| int length = array.length; | |
| if (!array.itemType.equals(String.class)) | |
| throw new ClientException("Not A String type."); | |
| String [] values = new String[length]; | |
| for (int i = 0; i < length; i++) { | |
| values[i] = (String)((BinaryStreamReader.ArrayValue) value).get(i); | |
| } | |
| return values; | |
| } | |
| throw new ClientException("Not ArrayValue type."); | |
| } | |
| @Override | |
| public String[] getStringArray(String colName) { | |
| try { | |
| return getPrimitiveArray(colName, String.class); | |
| } catch (ClassCastException | IllegalArgumentException e) { | |
| throw new ClientException("Value cannot be converted to an array of strings", e); | |
| } | |
| } |
| throw new ClientException("Not A String type."); | ||
| String [] values = new String[length]; | ||
| for (int i = 0; i < length; i++) { | ||
| values[i] = (String)((BinaryStreamReader.ArrayValue) value).get(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary cast in the loop when accessing array elements. The value is already cast to BinaryStreamReader.ArrayValue earlier.
| values[i] = (String)((BinaryStreamReader.ArrayValue) value).get(i); | |
| values[i] = (String)array.get(i); |
| /** | ||
| * | ||
| * @param colName | ||
| * @return | ||
| */ | ||
| short[] getShortArray(String colName); | ||
|
|
||
| /** | ||
| * | ||
| * @param colName | ||
| * @return | ||
| */ | ||
| String[] getStringArray(String colName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JavaDoc comments for the new methods are incomplete. Consider adding descriptions of what these methods do, similar to other array getter methods in the interface.
| /** | |
| * | |
| * @param colName | |
| * @return | |
| */ | |
| short[] getShortArray(String colName); | |
| /** | |
| * | |
| * @param colName | |
| * @return | |
| */ | |
| String[] getStringArray(String colName); | |
| /** | |
| * Reads column with name `colName` as a short array. | |
| * | |
| * @param colName | |
| * @return | |
| */ | |
| short[] getShortArray(String colName); | |
| /** | |
| * Reads column with name `colName` as a string array. | |
| * | |
| * @param colName | |
| * @return | |
| */ | |
| String[] getStringArray(String colName); |


Summary
Closes
Checklist
Delete items not relevant to your PR: