Skip to content

Commit

Permalink
Fix handling of nulls in Comparators, Fix #428
Browse files Browse the repository at this point in the history
  • Loading branch information
@minborg
minborg committed May 12, 2017
1 parent abfd158 commit 01bdf0d
Show file tree
Hide file tree
Showing 12 changed files with 162 additions and 48 deletions.
26 changes: 26 additions & 0 deletions runtime-parent/runtime-core/src/main/java/com/speedment/runtime/core/db/DbmsType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,30 @@ enum SubSelectAlias {
*/
SubSelectAlias getSubSelectAlias();

/**
* The sort by null order insertion. E.g. how is null first/last handled
*/
enum SortByNullOrderInsertion {
/*
* ORDER BY col IS NULL, col DESC
*/
PRE,
/*
* ORDER BY CASE WHEN col IS NULL THEN 0 ELSE 1 END, col DESC
*/
PRE_WITH_CASE,
/*
* ORDER BY col DESC NULLS FIRST
*/
POST;

}

/**
* Returns the SortByNullOrderInsertion mode for this database type.
*
* @return the SortByNullOrderInsertion mode for this database type
*/
SortByNullOrderInsertion getSortByNullOrderInsertion();

}
32 changes: 27 additions & 5 deletions ...om/speedment/runtime/core/internal/component/sql/optimizer/FilterSortedSkipOptimizer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import com.speedment.runtime.core.stream.Pipeline;
import com.speedment.runtime.core.stream.action.Action;
import com.speedment.runtime.field.comparator.FieldComparator;
import com.speedment.runtime.field.predicate.FieldPredicate;
import com.speedment.runtime.field.comparator.NullOrder;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
Expand Down Expand Up @@ -134,7 +134,8 @@ public <P extends Pipeline> P optimize(
requireNonNull(initialPipeline);
requireNonNull(info);
requireNonNull(query);
final DbmsType.SkipLimitSupport skipLimitSupport = info.getDbmsType().getSkipLimitSupport();
final DbmsType dbmsType = info.getDbmsType();
final DbmsType.SkipLimitSupport skipLimitSupport = dbmsType.getSkipLimitSupport();
final List<FilterAction<ENTITY>> filters = new ArrayList<>();
final List<SortedComparatorAction<ENTITY>> sorteds = new ArrayList<>();
final List<SkipAction<ENTITY>> skips = new ArrayList<>();
Expand All @@ -155,7 +156,7 @@ public <P extends Pipeline> P optimize(
.collect(toList());

final RenderResult rr = StreamTerminatorUtil.renderSqlWhere(
info.getDbmsType(),
dbmsType,
info.getSqlColumnNamer(),
info.getSqlDatabaseTypeFunction(),
predicates
Expand All @@ -175,20 +176,41 @@ public <P extends Pipeline> P optimize(
final FieldComparator<ENTITY, ?> fieldComparator = (FieldComparator<ENTITY, ?>) sortedAction.getComparator();
final ColumnIdentifier<ENTITY> columnIdentifier = fieldComparator.getField().identifier();

// SQL Server only allows distict columns in ORDER BY
// Some databases (e.g. SQL Server) only allows distinct columns in ORDER BY
if (columns.add(columnIdentifier)) {
if (!(i == (sorteds.size() - 1))) {
sql.append(", ");
}

boolean isReversed = fieldComparator.isReversed();
String fieldName = info.getSqlColumnNamer().apply(fieldComparator.getField());

final NullOrder effectiveNullOrder = isReversed
? fieldComparator.getNullOrder().reversed()
: fieldComparator.getNullOrder();

// Specify NullOrder pre column if nulls are first
if (effectiveNullOrder == NullOrder.FIRST) {
if (dbmsType.getSortByNullOrderInsertion() == DbmsType.SortByNullOrderInsertion.PRE) {
sql.append(fieldName).append("IS NOT NULL, ");
}
if (dbmsType.getSortByNullOrderInsertion() == DbmsType.SortByNullOrderInsertion.PRE_WITH_CASE) {
sql.append("CASE WHEN ").append(fieldName).append(" IS NULL THEN 0 ELSE 1 END, ");
}
}

sql.append(fieldName);
if (isReversed) {
sql.append(" DESC");
} else {
sql.append(" ASC");
}

// Specify NullOrder post column
if (effectiveNullOrder == NullOrder.FIRST && dbmsType.getSortByNullOrderInsertion() == DbmsType.SortByNullOrderInsertion.POST) {
sql.append(" NULLS FIRST");
}

}
}
}
Expand All @@ -200,7 +222,7 @@ public <P extends Pipeline> P optimize(
} else {
final long sumSkip = skips.stream().mapToLong(SkipAction::getSkip).sum();
final long minLimit = limits.stream().mapToLong(LimitAction::getLimit).min().orElse(Long.MAX_VALUE);
finalSql = info.getDbmsType()
finalSql = dbmsType
.applySkipLimit(sql.toString(), values, sumSkip, minLimit);
initialPipeline.removeIf(a -> filters.contains(a) || sorteds.contains(a) || skips.contains(a) || limits.contains(a));
}
Expand Down
5 changes: 5 additions & 0 deletions ...t/runtime-core/src/main/java/com/speedment/runtime/core/internal/db/AbstractDbmsType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,9 @@ public SubSelectAlias getSubSelectAlias() {
return SubSelectAlias.REQUIRED;
}

@Override
public SortByNullOrderInsertion getSortByNullOrderInsertion() {
return SortByNullOrderInsertion.PRE;
}

}
5 changes: 5 additions & 0 deletions ...e/src/main/java/com/speedment/runtime/core/internal/db/postgresql/PostgresqlDbmsType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ protected String getFieldEncloserEnd() {
}
}

@Override
public SortByNullOrderInsertion getSortByNullOrderInsertion() {
return SortByNullOrderInsertion.POST;
}

private final static class PostgresConnectionUrlGenerator implements ConnectionUrlGenerator {

@Override
Expand Down
1 change: 1 addition & 0 deletions ...n/java/com/speedment/runtime/core/internal/stream/builder/action/trait/HasComparator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
/**
*
* @author Per Minborg
* @param <T> type to compare
*/
public interface HasComparator<T> {

Expand Down
30 changes: 24 additions & 6 deletions ...-parent/runtime-field/src/main/java/com/speedment/runtime/field/comparator/NullOrder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,33 @@
*/
package com.speedment.runtime.field.comparator;

import java.util.EnumMap;
import java.util.Map;

/**
* Determines in if null values should be located before or
* after other values in a list of results.
*
* @author Per Minborg
* @since 2.2.0
* Determines in if null values should be located before or after other values
* in a list of results.
*
* @author Per Minborg
* @since 2.2.0
*/
public enum NullOrder {

NONE, FIRST, LAST
NONE, FIRST, LAST;

private static final Map<NullOrder, NullOrder> REVERSED_MAP = new EnumMap<>(NullOrder.class);

static {
REVERSED_MAP.put(NONE, NONE);
REVERSED_MAP.put(FIRST, LAST);
REVERSED_MAP.put(LAST, FIRST);
if (REVERSED_MAP.size() != NullOrder.values().length) {
throw new IllegalStateException("Missing mapping for NullOrder");
}
}

public NullOrder reversed() {
return REVERSED_MAP.get(this);
}

}
4 changes: 2 additions & 2 deletions ...runtime-field/src/main/java/com/speedment/runtime/field/internal/ComparableFieldImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public boolean isUnique() {

@Override
public FieldComparator<ENTITY, V> comparator() {
return new ReferenceFieldComparatorImpl<>(this, NullOrder.NONE);
return new ReferenceFieldComparatorImpl<>(this, NullOrder.LAST);
}

@Override
Expand All @@ -109,7 +109,7 @@ public FieldComparator<ENTITY, V> comparatorNullFieldsFirst() {

@Override
public FieldComparator<ENTITY, V> comparatorNullFieldsLast() {
return new ReferenceFieldComparatorImpl<>(this, NullOrder.LAST);
return comparator();
}

/*****************************************************************/
Expand Down
4 changes: 2 additions & 2 deletions ...eld/src/main/java/com/speedment/runtime/field/internal/ComparableForeignKeyFieldImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public boolean isUnique() {

@Override
public FieldComparator<ENTITY, V> comparator() {
return new ReferenceFieldComparatorImpl<>(this, NullOrder.NONE);
return new ReferenceFieldComparatorImpl<>(this, NullOrder.LAST);
}

@Override
Expand All @@ -136,7 +136,7 @@ public FieldComparator<ENTITY, V> comparatorNullFieldsFirst() {

@Override
public FieldComparator<ENTITY, V> comparatorNullFieldsLast() {
return new ReferenceFieldComparatorImpl<>(this, NullOrder.LAST);
return comparator();
}

/*****************************************************************/
Expand Down
4 changes: 2 additions & 2 deletions ...ent/runtime-field/src/main/java/com/speedment/runtime/field/internal/StringFieldImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public boolean isUnique() {

@Override
public FieldComparator<ENTITY, String> comparator() {
return new ReferenceFieldComparatorImpl<>(this, NullOrder.NONE);
return new ReferenceFieldComparatorImpl<>(this, NullOrder.LAST);
}

@Override
Expand All @@ -107,7 +107,7 @@ public FieldComparator<ENTITY, String> comparatorNullFieldsFirst() {

@Override
public FieldComparator<ENTITY, String> comparatorNullFieldsLast() {
return new ReferenceFieldComparatorImpl<>(this, NullOrder.LAST);
return comparator();
}

/*****************************************************************/
Expand Down
4 changes: 2 additions & 2 deletions ...e-field/src/main/java/com/speedment/runtime/field/internal/StringForeignKeyFieldImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public boolean isUnique() {
/**************************************************************************/
@Override
public FieldComparator<ENTITY, String> comparator() {
return new ReferenceFieldComparatorImpl<>(this, NullOrder.NONE);
return new ReferenceFieldComparatorImpl<>(this, NullOrder.LAST);
}

@Override
Expand All @@ -133,7 +133,7 @@ public FieldComparator<ENTITY, String> comparatorNullFieldsFirst() {

@Override
public FieldComparator<ENTITY, String> comparatorNullFieldsLast() {
return new ReferenceFieldComparatorImpl<>(this, NullOrder.LAST);
return comparator();
}

/**************************************************************************/
Expand Down
60 changes: 31 additions & 29 deletions ...runtime-field/src/main/java/com/speedment/runtime/field/trait/HasComparableOperators.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
*/
package com.speedment.runtime.field.trait;


import com.speedment.runtime.field.Field;
import com.speedment.runtime.field.comparator.FieldComparator;
import com.speedment.runtime.field.predicate.Inclusion;
Expand All @@ -30,16 +29,15 @@
* A representation of an Entity field that is a reference type (e.g.
* {@code Integer} and not {@code int}) and that implements {@link Comparable}.
*
* @param <ENTITY> the entity type
* @param <V> the field value type
* @param <ENTITY> the entity type
* @param <V> the field value type
*
* @author Per Minborg
* @author Emil Forslund
* @since 2.2.0
* @author Per Minborg
* @author Emil Forslund
* @since 2.2.0
*/

public interface HasComparableOperators<ENTITY, V extends Comparable<? super V>>
extends Field<ENTITY> {
public interface HasComparableOperators<ENTITY, V extends Comparable<? super V>>
extends Field<ENTITY> {

/**
* Returns a {@link Comparator} that will compare to this field using this
Expand All @@ -66,7 +64,12 @@ public interface HasComparableOperators<ENTITY, V extends Comparable<? super V>>
*
* @return a {@link Comparator} that will compare to this field using this
* fields natural order, null fields are sorted last
*
*
* @deprecated This method is the same as comparator(). Use that method
* instead. This method will be removed in coming API versions.
*/
@Deprecated
FieldComparator<ENTITY, V> comparatorNullFieldsLast();

/**
Expand All @@ -90,7 +93,7 @@ public interface HasComparableOperators<ENTITY, V extends Comparable<? super V>>
* this Field is <em>not equal</em> to the given value
*/
Predicate<ENTITY> notEqual(V value);

/**
* Returns a {@link java.util.function.Predicate} that will evaluate to
* {@code true}, if and only if this Field is <em>less than</em> the given
Expand All @@ -110,8 +113,8 @@ public interface HasComparableOperators<ENTITY, V extends Comparable<? super V>>
* {@code true}, if and only if this Field is <em>less than or equal</em>
* to the given value.
* <p>
* If the specified value is {@code null}, the returned predicate will
* only return {@code true} for {@code null} values.
* If the specified value is {@code null}, the returned predicate will only
* return {@code true} for {@code null} values.
*
* @param value to compare
* @return a Predicate that will evaluate to {@code true}, if and only if
Expand All @@ -122,13 +125,12 @@ public interface HasComparableOperators<ENTITY, V extends Comparable<? super V>>
/**
* Returns a {@link java.util.function.Predicate} that will evaluate to
* {@code true}, if and only if this Field is <em>greater than</em>
* the given value.
* If the specified value is {@code null}, the returned predicate will
* always return {@code false}.
* the given value. If the specified value is {@code null}, the returned
* predicate will always return {@code false}.
*
* @param value to compare
* @return a Predicate that will evaluate to {@code true}, if and only if
* this Field is <em>greater than</em> the given value
* @param value to compare
* @return a Predicate that will evaluate to {@code true}, if and only if
* this Field is <em>greater than</em> the given value
*/
Predicate<ENTITY> greaterThan(V value);

Expand All @@ -137,12 +139,12 @@ public interface HasComparableOperators<ENTITY, V extends Comparable<? super V>>
* {@code true}, if and only if this Field is <em>greater than or equal</em>
* to the given value.
* <p>
* If the specified value is {@code null}, the returned predicate will
* only return {@code true} for {@code null} values.
* If the specified value is {@code null}, the returned predicate will only
* return {@code true} for {@code null} values.
*
* @param value to compare
* @return a Predicate that will evaluate to {@code true}, if and only if
* this Field is <em>greater than or equal</em> to the given value
* @param value to compare
* @return a Predicate that will evaluate to {@code true}, if and only if
* this Field is <em>greater than or equal</em> to the given value
*/
Predicate<ENTITY> greaterOrEqual(V value);

Expand All @@ -154,11 +156,11 @@ public interface HasComparableOperators<ENTITY, V extends Comparable<? super V>>
* N.B. if the start value is greater or equal to the end value, then the
* returned Predicate will always evaluate to {@code false}.
*
* @param start to compare as a start value
* @param end to compare as an end value
* @return a Predicate that will evaluate to {@code true}, if and only if
* this Field is <em>between</em> the given values (inclusive the
* start value but exclusive the end value)
* @param start to compare as a start value
* @param end to compare as an end value
* @return a Predicate that will evaluate to {@code true}, if and only if
* this Field is <em>between</em> the given values (inclusive the start
* value but exclusive the end value)
*/
default Predicate<ENTITY> between(V start, V end) {
return between(start, end, Inclusion.START_INCLUSIVE_END_EXCLUSIVE);
Expand Down Expand Up @@ -283,4 +285,4 @@ default Predicate<ENTITY> notIn(V... values) {
* this Field is <em>not in</em> the given Set
*/
Predicate<ENTITY> notIn(Set<V> values);
}
}
Loading

0 comments on commit 01bdf0d

Please sign in to comment.