This is an automated email from the ASF dual-hosted git repository.
gian pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new 9436ee8a63 Nicer error message for CSV with no properties. (#14093)
9436ee8a63 is described below
commit 9436ee8a63b6ccfd668c45c5304154f09a9f9919
Author: Gian Merlino <[email protected]>
AuthorDate: Tue Apr 18 12:52:02 2023 -0700
Nicer error message for CSV with no properties. (#14093)
* Nicer error message for CSV with no properties.
* Take two.
* Adjustments from review, and test fixes.
* Fix test.
* Fix static check.
---
.../druid/data/input/impl/FlatTextInputFormat.java | 25 ++++---
.../main/java/org/apache/druid/indexer/Checks.java | 11 ++-
.../druid/data/input/impl/CsvInputFormatTest.java | 85 +++++++++++++++++++++-
.../data/input/impl/DelimitedInputFormatTest.java | 4 +-
.../java/org/apache/druid/indexer/ChecksTest.java | 15 +---
5 files changed, 110 insertions(+), 30 deletions(-)
diff --git
a/processing/src/main/java/org/apache/druid/data/input/impl/FlatTextInputFormat.java
b/processing/src/main/java/org/apache/druid/data/input/impl/FlatTextInputFormat.java
index bcd9eee5bd..b542fc23d1 100644
---
a/processing/src/main/java/org/apache/druid/data/input/impl/FlatTextInputFormat.java
+++
b/processing/src/main/java/org/apache/druid/data/input/impl/FlatTextInputFormat.java
@@ -22,10 +22,8 @@ package org.apache.druid.data.input.impl;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
import org.apache.druid.data.input.InputFormat;
-import org.apache.druid.indexer.Checks;
-import org.apache.druid.indexer.Property;
+import org.apache.druid.java.util.common.IAE;
import javax.annotation.Nullable;
import java.util.Collections;
@@ -52,14 +50,17 @@ public abstract class FlatTextInputFormat implements
InputFormat
this.columns = columns == null ? Collections.emptyList() : columns;
this.listDelimiter = listDelimiter;
this.delimiter = Preconditions.checkNotNull(delimiter, "delimiter");
- //noinspection ConstantConditions
if (columns == null || columns.isEmpty()) {
- this.findColumnsFromHeader = Checks.checkOneNotNullOrEmpty(
- ImmutableList.of(
- new Property<>("hasHeaderRow", hasHeaderRow),
- new Property<>("findColumnsFromHeader", findColumnsFromHeader)
- )
- ).getValue();
+ if (hasHeaderRow != null && findColumnsFromHeader != null) {
+ // User provided both hasHeaderRow and findColumnsFromHeader.
+ throw new IAE("Cannot accept both [findColumnsFromHeader] and
[hasHeaderRow]");
+ } else if (hasHeaderRow == null && findColumnsFromHeader == null) {
+ // User provided neither columns, nor one of the header-related
parameters.
+ throw new IAE("Either [columns] or [findColumnsFromHeader] must be
set");
+ } else {
+ // User provided one of hasHeaderRow or findColumnsFromHeader. Take
the one they provided.
+ this.findColumnsFromHeader = hasHeaderRow != null ? hasHeaderRow :
findColumnsFromHeader;
+ }
} else {
this.findColumnsFromHeader = findColumnsFromHeader == null ? false :
findColumnsFromHeader;
}
@@ -80,8 +81,8 @@ public abstract class FlatTextInputFormat implements
InputFormat
} else {
Preconditions.checkArgument(
this.findColumnsFromHeader,
- "If columns field is not set, the first row of your data must have
your header"
- + " and hasHeaderRow must be set to true."
+ "If [columns] is not set, the first row of your data must have your
header"
+ + " and [findColumnsFromHeader] must be set to true."
);
}
}
diff --git a/processing/src/main/java/org/apache/druid/indexer/Checks.java
b/processing/src/main/java/org/apache/druid/indexer/Checks.java
index 7153b74763..e287c2b4f7 100644
--- a/processing/src/main/java/org/apache/druid/indexer/Checks.java
+++ b/processing/src/main/java/org/apache/druid/indexer/Checks.java
@@ -22,6 +22,7 @@ package org.apache.druid.indexer;
import org.apache.druid.java.util.common.IAE;
import java.util.List;
+import java.util.stream.Collectors;
/**
* Various helper methods useful for checking the validity of arguments to
spec constructors.
@@ -36,12 +37,18 @@ public final class Checks
if (nonNullProperty == null) {
nonNullProperty = property;
} else {
- throw new IAE("At most one of %s must be present", properties);
+ throw new IAE(
+ "At most one of properties[%s] must be present",
+
properties.stream().map(Property::getName).collect(Collectors.toList())
+ );
}
}
}
if (nonNullProperty == null) {
- throw new IAE("At least one of %s must be present", properties);
+ throw new IAE(
+ "At least one of properties[%s] must be present",
+
properties.stream().map(Property::getName).collect(Collectors.toList())
+ );
}
return nonNullProperty;
}
diff --git
a/processing/src/test/java/org/apache/druid/data/input/impl/CsvInputFormatTest.java
b/processing/src/test/java/org/apache/druid/data/input/impl/CsvInputFormatTest.java
index 348b3eaa4b..5f5a52ef15 100644
---
a/processing/src/test/java/org/apache/druid/data/input/impl/CsvInputFormatTest.java
+++
b/processing/src/test/java/org/apache/druid/data/input/impl/CsvInputFormatTest.java
@@ -19,12 +19,16 @@
package org.apache.druid.data.input.impl;
+import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.apache.druid.data.input.InputFormat;
import org.apache.druid.testing.InitializedNullHandlingTest;
+import org.hamcrest.CoreMatchers;
+import org.hamcrest.MatcherAssert;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
+import org.junit.internal.matchers.ThrowableMessageMatcher;
import org.junit.rules.ExpectedException;
import java.io.IOException;
@@ -45,6 +49,83 @@ public class CsvInputFormatTest extends
InitializedNullHandlingTest
Assert.assertEquals(format, fromJson);
}
+ @Test
+ public void testDeserializeWithoutColumnsWithHasHeaderRow() throws
IOException
+ {
+ final ObjectMapper mapper = new ObjectMapper();
+ final CsvInputFormat inputFormat = (CsvInputFormat) mapper.readValue(
+ "{\"type\":\"csv\",\"hasHeaderRow\":true}",
+ InputFormat.class
+ );
+ Assert.assertTrue(inputFormat.isFindColumnsFromHeader());
+ }
+
+ @Test
+ public void testDeserializeWithoutColumnsWithFindColumnsFromHeaderTrue()
throws IOException
+ {
+ final ObjectMapper mapper = new ObjectMapper();
+ final CsvInputFormat inputFormat = (CsvInputFormat) mapper.readValue(
+ "{\"type\":\"csv\",\"findColumnsFromHeader\":true}",
+ InputFormat.class
+ );
+ Assert.assertTrue(inputFormat.isFindColumnsFromHeader());
+ }
+
+ @Test
+ public void testDeserializeWithoutColumnsWithFindColumnsFromHeaderFalse()
+ {
+ final ObjectMapper mapper = new ObjectMapper();
+ final JsonProcessingException e = Assert.assertThrows(
+ JsonProcessingException.class,
+ () -> mapper.readValue(
+ "{\"type\":\"csv\",\"findColumnsFromHeader\":false}",
+ InputFormat.class
+ )
+ );
+ MatcherAssert.assertThat(
+ e,
+ ThrowableMessageMatcher.hasMessage(CoreMatchers.startsWith(
+ "Cannot construct instance of
`org.apache.druid.data.input.impl.CsvInputFormat`, problem: "
+ + "If [columns] is not set, the first row of your data must have
your header and "
+ + "[findColumnsFromHeader] must be set to true."))
+ );
+ }
+
+ @Test
+ public void testDeserializeWithoutColumnsWithBothHeaderProperties()
+ {
+ final ObjectMapper mapper = new ObjectMapper();
+ final JsonProcessingException e = Assert.assertThrows(
+ JsonProcessingException.class,
+ () -> mapper.readValue(
+
"{\"type\":\"csv\",\"findColumnsFromHeader\":true,\"hasHeaderRow\":true}",
+ InputFormat.class
+ )
+ );
+ MatcherAssert.assertThat(
+ e,
+ ThrowableMessageMatcher.hasMessage(CoreMatchers.startsWith(
+ "Cannot construct instance of
`org.apache.druid.data.input.impl.CsvInputFormat`, problem: "
+ + "Cannot accept both [findColumnsFromHeader] and [hasHeaderRow]"))
+ );
+ }
+
+ @Test
+ public void testDeserializeWithoutAnyProperties()
+ {
+ final ObjectMapper mapper = new ObjectMapper();
+ final JsonProcessingException e = Assert.assertThrows(
+ JsonProcessingException.class,
+ () -> mapper.readValue("{\"type\":\"csv\"}", InputFormat.class)
+ );
+ MatcherAssert.assertThat(
+ e,
+ ThrowableMessageMatcher.hasMessage(CoreMatchers.startsWith(
+ "Cannot construct instance of
`org.apache.druid.data.input.impl.CsvInputFormat`, problem: "
+ + "Either [columns] or [findColumnsFromHeader] must be set"))
+ );
+ }
+
@Test
public void testComma()
{
@@ -79,7 +160,7 @@ public class CsvInputFormatTest extends
InitializedNullHandlingTest
public void testMissingFindColumnsFromHeaderWithMissingColumnsThrowingError()
{
expectedException.expect(IllegalArgumentException.class);
- expectedException.expectMessage("At least one of
[Property{name='hasHeaderRow', value=null}");
+ expectedException.expectMessage("Either [columns] or
[findColumnsFromHeader] must be set");
new CsvInputFormat(null, null, null, null, 0);
}
@@ -94,7 +175,7 @@ public class CsvInputFormatTest extends
InitializedNullHandlingTest
public void testHasHeaderRowWithMissingFindColumnsThrowingError()
{
expectedException.expect(IllegalArgumentException.class);
- expectedException.expectMessage("At most one of
[Property{name='hasHeaderRow', value=true}");
+ expectedException.expectMessage("Cannot accept both
[findColumnsFromHeader] and [hasHeaderRow]");
new CsvInputFormat(null, null, true, false, 0);
}
diff --git
a/processing/src/test/java/org/apache/druid/data/input/impl/DelimitedInputFormatTest.java
b/processing/src/test/java/org/apache/druid/data/input/impl/DelimitedInputFormatTest.java
index a21e34d8cc..35f35ab9cc 100644
---
a/processing/src/test/java/org/apache/druid/data/input/impl/DelimitedInputFormatTest.java
+++
b/processing/src/test/java/org/apache/druid/data/input/impl/DelimitedInputFormatTest.java
@@ -107,7 +107,7 @@ public class DelimitedInputFormatTest
public void testMissingFindColumnsFromHeaderWithMissingColumnsThrowingError()
{
expectedException.expect(IllegalArgumentException.class);
- expectedException.expectMessage("At least one of
[Property{name='hasHeaderRow', value=null}");
+ expectedException.expectMessage("Either [columns] or
[findColumnsFromHeader] must be set");
new DelimitedInputFormat(null, null, "delim", null, null, 0);
}
@@ -129,7 +129,7 @@ public class DelimitedInputFormatTest
public void testHasHeaderRowWithMissingFindColumnsThrowingError()
{
expectedException.expect(IllegalArgumentException.class);
- expectedException.expectMessage("At most one of
[Property{name='hasHeaderRow', value=true}");
+ expectedException.expectMessage("Cannot accept both
[findColumnsFromHeader] and [hasHeaderRow]");
new DelimitedInputFormat(null, null, "delim", true, false, 0);
}
diff --git a/processing/src/test/java/org/apache/druid/indexer/ChecksTest.java
b/processing/src/test/java/org/apache/druid/indexer/ChecksTest.java
index 8a74543540..9f10ad58ee 100644
--- a/processing/src/test/java/org/apache/druid/indexer/ChecksTest.java
+++ b/processing/src/test/java/org/apache/druid/indexer/ChecksTest.java
@@ -105,10 +105,7 @@ public class ChecksTest
new Property<>("p4", Collections.emptyList())
);
exception.expect(IllegalArgumentException.class);
- exception.expectMessage(
- "At most one of [Property{name='p1', value=null}, Property{name='p2',
value=2}, Property{name='p3', value=3}, "
- + "Property{name='p4', value=[]}] must be present"
- );
+ exception.expectMessage("At most one of properties[[p1, p2, p3, p4]] must
be present");
Checks.checkOneNotNullOrEmpty(properties);
}
@@ -122,10 +119,7 @@ public class ChecksTest
new Property<>("p4", Lists.newArrayList(1, 2))
);
exception.expect(IllegalArgumentException.class);
- exception.expectMessage(
- "At most one of [Property{name='p1', value=null}, Property{name='p2',
value=2}, Property{name='p3', value=null}, "
- + "Property{name='p4', value=[1, 2]}] must be present"
- );
+ exception.expectMessage("At most one of properties[[p1, p2, p3, p4]] must
be present");
Checks.checkOneNotNullOrEmpty(properties);
}
@@ -139,10 +133,7 @@ public class ChecksTest
new Property<>("p4", null)
);
exception.expect(IllegalArgumentException.class);
- exception.expectMessage(
- "At least one of [Property{name='p1', value=null}, Property{name='p2',
value=null}, "
- + "Property{name='p3', value=null}, Property{name='p4', value=null}]
must be present"
- );
+ exception.expectMessage("At least one of properties[[p1, p2, p3, p4]] must
be present");
Checks.checkOneNotNullOrEmpty(properties);
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
