SQL: Fix exception with OR of impossible filters. (#7707)

Fixes #7671.
This commit is contained in:
Gian Merlino 2019-05-21 11:32:09 -07:00 committed by Fangjin Yang
parent b6941551ae
commit bcea05e4e8
2 changed files with 66 additions and 45 deletions

View File

@ -118,48 +118,23 @@ public class CombineAndSimplifyBounds extends BottomUpTransform
* Simplify BoundDimFilters that are children of an OR or an AND. * Simplify BoundDimFilters that are children of an OR or an AND.
* *
* @param children the filters * @param children the filters
* @param disjunction true for disjunction, false for conjunction * @param disjunction true for OR, false for AND
* *
* @return simplified filters * @return simplified filters
*/ */
private static DimFilter doSimplify(final List<DimFilter> children, boolean disjunction) private static DimFilter doSimplify(final List<DimFilter> children, boolean disjunction)
{ {
// Copy children list // Copy the list of child filters. We'll modify the copy and eventually return it.
final List<DimFilter> newChildren = Lists.newArrayList(children); final List<DimFilter> newChildren = Lists.newArrayList(children);
// Group Bound filters by dimension, extractionFn, and comparator and compute a RangeSet for each one. // Group Bound filters by dimension, extractionFn, and comparator and compute a RangeSet for each one.
final Map<BoundRefKey, List<BoundDimFilter>> bounds = new HashMap<>(); final Map<BoundRefKey, List<BoundDimFilter>> bounds = new HashMap<>();
final Iterator<DimFilter> iterator = newChildren.iterator(); for (final DimFilter child : newChildren) {
while (iterator.hasNext()) { if (child instanceof BoundDimFilter) {
final DimFilter child = iterator.next();
if (child.equals(Filtration.matchNothing())) {
// Child matches nothing, equivalent to FALSE
// OR with FALSE => ignore
// AND with FALSE => always false, short circuit
if (disjunction) {
iterator.remove();
} else {
return Filtration.matchNothing();
}
} else if (child.equals(Filtration.matchEverything())) {
// Child matches everything, equivalent to TRUE
// OR with TRUE => always true, short circuit
// AND with TRUE => ignore
if (disjunction) {
return Filtration.matchEverything();
} else {
iterator.remove();
}
} else if (child instanceof BoundDimFilter) {
final BoundDimFilter bound = (BoundDimFilter) child; final BoundDimFilter bound = (BoundDimFilter) child;
final BoundRefKey boundRefKey = BoundRefKey.from(bound); final BoundRefKey boundRefKey = BoundRefKey.from(bound);
List<BoundDimFilter> filterList = bounds.get(boundRefKey); final List<BoundDimFilter> filterList = bounds.computeIfAbsent(boundRefKey, k -> new ArrayList<>());
if (filterList == null) {
filterList = new ArrayList<>();
bounds.put(boundRefKey, filterList);
}
filterList.add(bound); filterList.add(bound);
} }
} }
@ -184,25 +159,13 @@ public class CombineAndSimplifyBounds extends BottomUpTransform
if (rangeSet.asRanges().isEmpty()) { if (rangeSet.asRanges().isEmpty()) {
// range set matches nothing, equivalent to FALSE // range set matches nothing, equivalent to FALSE
// OR with FALSE => ignore
// AND with FALSE => always false, short circuit
if (disjunction) {
newChildren.add(Filtration.matchNothing()); newChildren.add(Filtration.matchNothing());
} else {
return Filtration.matchNothing();
}
} }
for (final Range<BoundValue> range : rangeSet.asRanges()) { for (final Range<BoundValue> range : rangeSet.asRanges()) {
if (!range.hasLowerBound() && !range.hasUpperBound()) { if (!range.hasLowerBound() && !range.hasUpperBound()) {
// range matches all, equivalent to TRUE // range matches all, equivalent to TRUE
// AND with TRUE => ignore
// OR with TRUE => always true; short circuit
if (disjunction) {
return Filtration.matchEverything();
} else {
newChildren.add(Filtration.matchEverything()); newChildren.add(Filtration.matchEverything());
}
} else { } else {
newChildren.add(Bounds.toFilter(boundRefKey, range)); newChildren.add(Bounds.toFilter(boundRefKey, range));
} }
@ -210,8 +173,44 @@ public class CombineAndSimplifyBounds extends BottomUpTransform
} }
} }
// Finally: Go through newChildren, removing or potentially exiting early based on TRUE / FALSE marker filters.
Preconditions.checkState(newChildren.size() > 0, "newChildren.size > 0"); Preconditions.checkState(newChildren.size() > 0, "newChildren.size > 0");
if (newChildren.size() == 1) {
final Iterator<DimFilter> iterator = newChildren.iterator();
while (iterator.hasNext()) {
final DimFilter newChild = iterator.next();
if (Filtration.matchNothing().equals(newChild)) {
// Child matches nothing, equivalent to FALSE
// OR with FALSE => ignore
// AND with FALSE => always false, short circuit
if (disjunction) {
iterator.remove();
} else {
return Filtration.matchNothing();
}
} else if (Filtration.matchEverything().equals(newChild)) {
// Child matches everything, equivalent to TRUE
// OR with TRUE => always true, short circuit
// AND with TRUE => ignore
if (disjunction) {
return Filtration.matchEverything();
} else {
iterator.remove();
}
}
}
if (newChildren.isEmpty()) {
// If "newChildren" is empty at this point, it must have consisted entirely of TRUE / FALSE marker filters.
if (disjunction) {
// Must have been all FALSE filters (the only kind we would have removed above).
return Filtration.matchNothing();
} else {
// Must have been all TRUE filters (the only kind we would have removed above).
return Filtration.matchEverything();
}
} else if (newChildren.size() == 1) {
return newChildren.get(0); return newChildren.get(0);
} else { } else {
return disjunction ? new OrDimFilter(newChildren) : new AndDimFilter(newChildren); return disjunction ? new OrDimFilter(newChildren) : new AndDimFilter(newChildren);

View File

@ -1891,6 +1891,28 @@ public class CalciteQueryTest extends BaseCalciteQueryTest
); );
} }
@Test
public void testGroupByNothingWithImpossibleTimeFilter() throws Exception
{
// Regression test for https://github.com/apache/incubator-druid/issues/7671
testQuery(
"SELECT COUNT(*) FROM druid.foo\n"
+ "WHERE FLOOR(__time TO DAY) = TIMESTAMP '2000-01-02 01:00:00'\n"
+ "OR FLOOR(__time TO DAY) = TIMESTAMP '2000-01-02 02:00:00'",
ImmutableList.of(
Druids.newTimeseriesQueryBuilder()
.dataSource(CalciteTests.DATASOURCE1)
.intervals(querySegmentSpec())
.granularity(Granularities.ALL)
.aggregators(aggregators(new CountAggregatorFactory("a0")))
.context(TIMESERIES_CONTEXT_DEFAULT)
.build()
),
ImmutableList.of()
);
}
@Test @Test
public void testGroupByOneColumnWithLiterallyFalseFilter() throws Exception public void testGroupByOneColumnWithLiterallyFalseFilter() throws Exception
{ {