Skip to content

Handle missing values for delimited text files when Nullhandling is enabled #8779

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.

Sign up for GitHub

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

Merged
clintropolis merged 10 commits into from
Nov 20, 2019
Merged

Handle missing values for delimited text files when Nullhandling is enabled #8779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0562c99
Handle missing values
Oct 29, 2019
e671219
Merge branch 'master' of https://github.com/druid-io/druid into handl…
Nov 1, 2019
e69c221
Fix multi value tests
Nov 1, 2019
3d190fd
Fix firehose tests
Nov 1, 2019
0ba91e1
Merge branch 'master' of https://github.com/druid-io/druid into handl…
Nov 7, 2019
d123ffa
Merge branch 'master' of https://github.com/druid-io/druid into handl…
Nov 8, 2019
769e232
Merge branch 'master' of https://github.com/druid-io/druid into handl…
Nov 8, 2019
4cef922
Merge branch 'master' of https://github.com/druid-io/druid into handl…
Nov 13, 2019
375ea1f
Merge branch 'master' of https://github.com/druid-io/druid into handl…
Nov 19, 2019
6cb53b4
Fix conflicts
Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion core/src/main/java/org/apache/druid/java/util/common/parsers/CSVParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@

import com.google.common.annotations.VisibleForTesting;
import com.opencsv.RFC4180Parser;
import com.opencsv.RFC4180ParserBuilder;
import com.opencsv.enums.CSVReaderNullFieldIndicator;
import org.apache.druid.common.config.NullHandling;

import javax.annotation.Nullable;
import java.io.IOException;
Expand All @@ -29,7 +32,10 @@

public class CSVParser extends AbstractFlatTextFormatParser
{
private final RFC4180Parser parser = new RFC4180Parser();
private final RFC4180Parser parser = NullHandling.replaceWithDefault()
? new RFC4180Parser()
: new RFC4180ParserBuilder().withFieldAsNull(
Copy link
Copy Markdown
Member

@clintropolis clintropolis Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this could just be the all the time mode, since I think it wouldn't really change anything for the default mode either since the values would be coerced to ''.

Copy link
Copy Markdown
Contributor Author

@a2l007 a2l007 Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention was to preserve backward compatibility but thinking about it, the empty separators flag might not impact the default mode. I'll do a few tests and raise a PR to make CSVReaderNullFieldIndicator.EMPTY_SEPARATORSthe default mode if it goes well.

CSVReaderNullFieldIndicator.EMPTY_SEPARATORS).build();

public CSVParser(
@Nullable final String listDelimiter,
Expand Down
8 changes: 7 additions & 1 deletion core/src/main/java/org/apache/druid/java/util/common/parsers/DelimitedParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Splitter;
import org.apache.druid.common.config.NullHandling;

import javax.annotation.Nullable;
import java.util.ArrayList;
Expand Down Expand Up @@ -94,7 +95,12 @@ private List<String> splitToList(String input)
List<String> result = new ArrayList<String>();

while (iterator.hasNext()) {
result.add(iterator.next());
String splitValue = iterator.next();
if (!NullHandling.replaceWithDefault() && splitValue.isEmpty()) {
result.add(null);
} else {
result.add(splitValue);
}
}

return Collections.unmodifiableList(result);
Expand Down
14 changes: 14 additions & 0 deletions core/src/test/java/org/apache/druid/java/util/common/parsers/FlatTextFormatParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,20 @@ public void testWithoutStartFileFromBeginning()
parser.parseToMap(body[0]);
}

@Test
public void testWithNullValues()
{
final Parser<String, Object> parser = PARSER_FACTORY.get(format, true, 0);
parser.startFileFromBeginning();
final String[] body = new String[]{
concat(format, "time", "value1", "value2"),
concat(format, "hello", "world", "")
};
Assert.assertNull(parser.parseToMap(body[0]));
final Map<String, Object> jsonMap = parser.parseToMap(body[1]);
Assert.assertNull(jsonMap.get("value2"));
}

private static class FlatTextFormatParserFactory
{
public Parser<String, Object> get(FlatTextFormat format)
Expand Down
4 changes: 1 addition & 3 deletions ...service/src/test/java/org/apache/druid/indexing/overlord/sampler/FirehoseSamplerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,9 +814,7 @@ private ParseSpec getParseSpec(TimestampSpec timestampSpec, DimensionsSpec dimen
private String getUnparseableTimestampString()
{
return ParserType.STR_CSV.equals(parserType)
? (USE_DEFAULT_VALUE_FOR_NULL
? "Unparseable timestamp found! Event: {t=bad_timestamp, dim1=foo, dim2=null, met1=6}"
: "Unparseable timestamp found! Event: {t=bad_timestamp, dim1=foo, dim2=, met1=6}")
? "Unparseable timestamp found! Event: {t=bad_timestamp, dim1=foo, dim2=null, met1=6}"
Copy link
Copy Markdown
Contributor Author

@a2l007 a2l007 Nov 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nishantmonu51 Please let me know if you think there is a better way to do this.
With the CSVParser fix in this PR, the way to specify empty values for csv files would be ""
One of the test data samples is bad_timestamp,foo,,6 and dim2 would be null regardless of whether null handling is enabled or not.

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be ok. @nishantmonu51 do you have something in your mind?

: "Unparseable timestamp found! Event: {t=bad_timestamp, dim1=foo, met1=6}";
}

Expand Down
2 changes: 1 addition & 1 deletion processing/src/test/java/org/apache/druid/query/MultiValuedDimensionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public void setup() throws Exception
"2011-01-12T00:00:00.000Z,product_1,t1\tt2\tt3,u1\tu2",
"2011-01-13T00:00:00.000Z,product_2,t3\tt4\tt5,u3\tu4",
"2011-01-14T00:00:00.000Z,product_3,t5\tt6\tt7,u1\tu5",
"2011-01-14T00:00:00.000Z,product_4,,u2"
"2011-01-14T00:00:00.000Z,product_4,\"\",u2"
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this change for?

Copy link
Copy Markdown
Member

@clintropolis clintropolis Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is to preserve the original test, since previously csv parser would always produce empty strings here, but after this change in null compatible mode it will produce a null

Copy link
Copy Markdown
Contributor

@jihoonson jihoonson Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see.

Copy link
Copy Markdown
Contributor Author

@a2l007 a2l007 Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying this. I was too late 💤

};

for (String row : rows) {
Expand Down