Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
abc7cbb
Part 2 of changes for SQL Compatible Null Handling
nishantmonu51 Jul 6, 2018
6a6d35a
Review comments - break lines longer than 120 characters
nishantmonu51 Jul 9, 2018
711fabe
Merge remote-tracking branch 'upstream/master' into null-handling-pr
nishantmonu51 Jul 24, 2018
cd2703e
review comments
nishantmonu51 Jul 24, 2018
d4a0584
review comments
nishantmonu51 Jul 26, 2018
dac0096
fix license
nishantmonu51 Jul 26, 2018
9071c2e
fix test failure
nishantmonu51 Jul 26, 2018
c839d63
fix CalciteQueryTest failure
nishantmonu51 Jul 27, 2018
aaa9d43
Null Handling - Review comments
nishantmonu51 Jul 27, 2018
7963038
Merge remote-tracking branch 'upstream/master' into null-handling-merge
nishantmonu51 Jul 31, 2018
7cbebdc
review comments
nishantmonu51 Jul 31, 2018
e1d37bd
Merge review comments after testing in another branch
nishantmonu51 Jul 31, 2018
6022648
review comments
nishantmonu51 Jul 31, 2018
7335a2f
Merge remote-tracking branch 'upstream/master' into null-handling-pr
nishantmonu51 Aug 1, 2018
36c0dc8
fix checkstyle
nishantmonu51 Aug 1, 2018
6c2102f
fix checkstyle
nishantmonu51 Aug 1, 2018
4458b41
remove unrelated change
nishantmonu51 Aug 1, 2018
a458ade
fix test failure
nishantmonu51 Aug 1, 2018
71904cd
fix failing test
nishantmonu51 Aug 1, 2018
9fd19e9
fix travis failures
nishantmonu51 Aug 1, 2018
1437081
Merge remote-tracking branch 'remotes/upstream/master' into null-hand…
nishantmonu51 Aug 2, 2018
312c5db
fix merge conflicts
nishantmonu51 Aug 2, 2018
2ae1007
Make StringLast and StringFirst aggregators nullable and fix travis f…
nishantmonu51 Aug 2, 2018
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
30 changes: 30 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ matrix:
- unset _JAVA_OPTIONS
script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -B -Pparallel-test -Dmaven.fork.count=2 -pl processing

# processing module tests with SQL Compatibility enabled
- sudo: false
env:
- NAME="processing module test with SQL Compatibility"
install: echo "MAVEN_OPTS='-Xmx3000m'" > ~/.mavenrc && mvn install -q -ff -DskipTests -B
before_script:
- unset _JAVA_OPTIONS
script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -B -Pparallel-test -Dmaven.fork.count=2 -Ddruid.generic.useDefaultValueForNull=false -pl processing

# server module test
- sudo: false
env:
Expand All @@ -43,6 +52,17 @@ matrix:
# Server module test is run without the parallel-test option because it's memory sensitive and often fails with that option.
script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -B -pl server

# server module test with SQL Compatibility enabled
- sudo: false
env:
- NAME="server module test with SQL Compatibility enabled"
install: echo "MAVEN_OPTS='-Xmx3000m'" > ~/.mavenrc && mvn install -q -ff -DskipTests -B
before_script:
- unset _JAVA_OPTIONS
# Server module test is run without the parallel-test option because it's memory sensitive and often fails with that option.
script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -B -pl server -Ddruid.generic.useDefaultValueForNull=false


# other modules test
- sudo: false
env:
Expand All @@ -53,6 +73,16 @@ matrix:
- unset _JAVA_OPTIONS
script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -B -Pparallel-test -Dmaven.fork.count=2 -pl '!processing,!server'

# other modules test with SQL Compatibility enabled
- sudo: false
env:
- NAME="other modules test with SQL Compatibility"
- AWS_REGION=us-east-1 # set a aws region for unit tests
install: echo "MAVEN_OPTS='-Xmx3000m'" > ~/.mavenrc && mvn install -q -ff -DskipTests -B
before_script:
- unset _JAVA_OPTIONS
script: echo "MAVEN_OPTS='-Xmx512m'" > ~/.mavenrc && mvn test -B -Pparallel-test -Dmaven.fork.count=2 -Ddruid.generic.useDefaultValueForNull=false -pl '!processing,!server'

# run integration tests
- sudo: required
services:
Expand Down
5 changes: 4 additions & 1 deletion api/src/main/java/io/druid/data/input/Rows.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Maps;
import com.google.common.primitives.Longs;
import io.druid.common.config.NullHandling;
import io.druid.java.util.common.StringUtils;
import io.druid.java.util.common.parsers.ParseException;

import javax.annotation.Nullable;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -92,10 +94,11 @@ public static List<String> objectToStrings(final Object inputValue)
* @throws NullPointerException if the string is null
* @throws ParseException if the column cannot be converted to a number
*/
@Nullable
public static Number objectToNumber(final String name, final Object inputValue)
{
if (inputValue == null) {
return Rows.LONG_ZERO;
return NullHandling.defaultLongValue();
}

if (inputValue instanceof Number) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@

import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.base.Predicate;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.io.Files;
import io.druid.benchmark.datagen.BenchmarkDataGenerator;
import io.druid.benchmark.datagen.BenchmarkSchemaInfo;
import io.druid.benchmark.datagen.BenchmarkSchemas;
import io.druid.common.config.NullHandling;
import io.druid.data.input.InputRow;
import io.druid.hll.HyperLogLogHash;
import io.druid.jackson.DefaultObjectMapper;
Expand Down Expand Up @@ -557,7 +557,7 @@ public Filter toFilter()
if (extractionFn == null) {
return new NoBitmapSelectorFilter(dimension, value);
} else {
final String valueOrNull = Strings.emptyToNull(value);
final String valueOrNull = NullHandling.emptyToNullIfNeeded(value);

final DruidPredicateFactory predicateFactory = new DruidPredicateFactory()
{
Expand Down
11 changes: 10 additions & 1 deletion codestyle/checkstyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@
<property name="illegalPattern" value="true"/>
<property name="message" value="Use java.lang.Primitive.BYTES instead."/>
</module>

<!-- This regex should be replaced with an IntelliJ inspection when teamcity.jetbrains.com updates to at least IntelliJ 2018.1 (currently it uses 2017.2) -->
<module name="Regexp">
<property name="format" value='[a-z][a-zA-Z0-9_]*\.equals\((\"|[A-Z_]+\))'/>
Expand All @@ -184,5 +183,15 @@
<property name="illegalPattern" value="true"/>
<property name="message" value='Use toArray(new Object[0]) instead'/>
</module>
<module name="Regexp">
<property name="format" value="Strings.emptyToNull"/>
<property name="illegalPattern" value="true"/>
<property name="message" value="Use io.druid.common.config.NullHandling.emptyToNullIfNeeded instead"/>
</module>
<module name="Regexp">
<property name="format" value="Strings.nullToEmpty"/>
<property name="illegalPattern" value="true"/>
<property name="message" value="Use io.druid.common.config.NullHandling.nullToEmptyIfNeeded instead"/>
</module>
</module>
</module>
4 changes: 2 additions & 2 deletions common/src/main/java/io/druid/math/expr/Evals.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

package io.druid.math.expr;

import com.google.common.base.Strings;
import io.druid.common.config.NullHandling;
import io.druid.java.util.common.logger.Logger;

import java.util.Arrays;
Expand Down Expand Up @@ -83,6 +83,6 @@ public static boolean asBoolean(double x)

public static boolean asBoolean(String x)
{
return !Strings.isNullOrEmpty(x) && Boolean.valueOf(x);
return !NullHandling.isNullOrEquivalent(x) && Boolean.valueOf(x);
}
}
14 changes: 13 additions & 1 deletion common/src/main/java/io/druid/math/expr/Expr.java
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,9 @@ class UnaryMinusExpr extends UnaryExpr
public ExprEval eval(ObjectBinding bindings)
{
ExprEval ret = expr.eval(bindings);
if (NullHandling.sqlCompatible() && (ret.value() == null)) {
return ExprEval.of(null);
}
if (ret.type() == ExprType.LONG) {
return ExprEval.of(-ret.asLong());
}
Expand Down Expand Up @@ -307,6 +310,9 @@ class UnaryNotExpr extends UnaryExpr
public ExprEval eval(ObjectBinding bindings)
{
ExprEval ret = expr.eval(bindings);
if (NullHandling.sqlCompatible() && (ret.value() == null)) {
return ExprEval.of(null);
}
// conforming to other boolean-returning binary operators
ExprType retType = ret.type() == ExprType.DOUBLE ? ExprType.DOUBLE : ExprType.LONG;
return ExprEval.of(!ret.asBoolean(), retType);
Expand Down Expand Up @@ -365,15 +371,21 @@ public ExprEval eval(ObjectBinding bindings)

// Result of any Binary expressions is null if any of the argument is null.
// e.g "select null * 2 as c;" or "select null + 1 as c;" will return null as per Standard SQL spec.
if (NullHandling.sqlCompatible() && (leftVal.isNull() || rightVal.isNull())) {
if (NullHandling.sqlCompatible() && (leftVal.value() == null || rightVal.value() == null)) {
return ExprEval.of(null);
}

if (leftVal.type() == ExprType.STRING && rightVal.type() == ExprType.STRING) {
return evalString(leftVal.asString(), rightVal.asString());
} else if (leftVal.type() == ExprType.LONG && rightVal.type() == ExprType.LONG) {
if (NullHandling.sqlCompatible() && (leftVal.isNumericNull() || rightVal.isNumericNull())) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wonder why we check this here instead of changing evalLong() or evalDouble to return a nullable Long or Double, respectively. Is this to avoid boxing/unboxing? If so, I wonder how much it affects to the performance.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not sure about how much performance effect boxing/unboxing will have, in this form atleast the change is on safer side and does not affect performance for non-sql case.
We can run more benchmarks in a follow up PR and change this api to return boxed values if the performance diff is not much.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks.

return ExprEval.of(null);
}
return ExprEval.of(evalLong(leftVal.asLong(), rightVal.asLong()));
} else {
if (NullHandling.sqlCompatible() && (leftVal.isNumericNull() || rightVal.isNumericNull())) {
return ExprEval.of(null);
}
return ExprEval.of(evalDouble(leftVal.asDouble(), rightVal.asDouble()));
}
}
Expand Down
Loading