-
Notifications
You must be signed in to change notification settings - Fork 636
[jdbc-v2,client-v2] Fix format selection and document how to #3101
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
base: main
Are you sure you want to change the base?
Changes from all commits
0c152ad
b3d083f
8caf953
503def4
b0e1fe3
0c35abd
97f7bab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package com.clickhouse.data; | ||
|
|
||
| import org.testng.Assert; | ||
| import org.testng.annotations.Test; | ||
|
|
||
| public class ClickHouseFormatTest { | ||
|
|
||
| @Test(groups = { "unit" }) | ||
| public void testFromStringNullAndEmpty() { | ||
| Assert.assertNull(ClickHouseFormat.fromString(null)); | ||
| Assert.assertNull(ClickHouseFormat.fromString("")); | ||
| Assert.assertNull(ClickHouseFormat.fromString(" ")); | ||
| } | ||
|
|
||
| @Test(groups = { "unit" }) | ||
| public void testFromStringValid() { | ||
| Assert.assertEquals(ClickHouseFormat.fromString("CSV"), ClickHouseFormat.CSV); | ||
| Assert.assertEquals(ClickHouseFormat.fromString("csv"), ClickHouseFormat.CSV); | ||
| Assert.assertEquals(ClickHouseFormat.fromString(" jsoneachrow "), ClickHouseFormat.JSONEachRow); | ||
| Assert.assertEquals(ClickHouseFormat.fromString("RowBinaryWithNamesAndTypes"), ClickHouseFormat.RowBinaryWithNamesAndTypes); | ||
| Assert.assertEquals(ClickHouseFormat.fromString("rowbinarywithnamesandtypes"), ClickHouseFormat.RowBinaryWithNamesAndTypes); | ||
| } | ||
|
|
||
| @Test(groups = { "unit" }) | ||
| public void testFromStringInvalid() { | ||
| Assert.expectThrows(IllegalArgumentException.class, () -> ClickHouseFormat.fromString("invalid_format_name_123")); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,7 +125,7 @@ public enum ClientConfigProperties { | |
|
|
||
| RETRY_ON_FAILURE("retry", Integer.class, "3"), | ||
|
|
||
| INPUT_OUTPUT_FORMAT("format", ClickHouseFormat.class), | ||
| INPUT_OUTPUT_FORMAT("format", ClickHouseFormat.class, ClickHouseFormat.RowBinaryWithNamesAndTypes.name()), | ||
|
|
||
| MAX_THREADS_PER_CLIENT("max_threads_per_client", Integer.class, "0"), | ||
|
|
||
|
|
@@ -347,9 +347,20 @@ public Object parseValue(String value) { | |
| } | ||
|
|
||
| if (valueType.isEnum()) { | ||
| String configValue = value.trim(); | ||
| if (configValue.isEmpty()) { | ||
| return null; | ||
| } | ||
| if (valueType.equals(ClickHouseFormat.class)) { | ||
| try { | ||
| return ClickHouseFormat.fromString(configValue); | ||
| } catch (IllegalArgumentException e) { | ||
| return configValue; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unknown formats crash query executionHigh Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit 97f7bab. Configure here. |
||
| } | ||
| Object[] constants = valueType.getEnumConstants(); | ||
| for (Object constant : constants) { | ||
| if (constant.toString().equals(value)) { | ||
| if (constant.toString().equalsIgnoreCase(configValue)) { | ||
| return constant; | ||
| } | ||
| } | ||
|
|
@@ -395,7 +406,9 @@ public static Map<String, Object> parseConfigMap(Map<String, String> configMap) | |
| default: | ||
| parsedValue = config.parseValue(value); | ||
| } | ||
| parsedConfig.put(config.getKey(), parsedValue); | ||
| if (parsedValue != null) { | ||
| parsedConfig.put(config.getKey(), parsedValue); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||


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.
Since this becomes permanent public API in 0.11.0, I'd settle its shape before merging:
ClickHouseFormat.valueOfis case-sensitive, soqueryFormat("csv")throws whilesetOption("format", "csv")now succeeds thanks to the case-insensitive parsing added in this PR.nullthrows an NPE, so the method cannot express the new "send no format header" mode this PR introduces; callers have to fall back tosetOption(INPUT_OUTPUT_FORMAT.getKey(), null).No enum constant ...message rather than aClientMisconfigurationException.Suggestion: take
ClickHouseFormatinstead ofString, withnullmeaning "no header", e.g.queryFormat(ClickHouseFormat format)storingformat == null ? null : format.name(). Alternatively route the string throughINPUT_OUTPUT_FORMAT.parseValue(...)so both entry points agree. Either way a test fornulland for an invalid value would be good; the only coverage today is theCSVhappy path inClientTests.testDefaultSettings.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.
Using
ClickHouseFormatrequire us to update code each time new format added to ClickHouse.So I'm thinking to fix the issue with that.