Skip to content
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.

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

Fix duplicate results and failures of queries with indexes and query parameters and improve index scan generation [HZ-3014] [HZ-3013] #25527

Merged
merged 4 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,9 @@ static IndexIterationPointer[] indexFilterToPointers(
boolean descending,
ExpressionEvalContext evalContext
) {
ArrayList<IndexIterationPointer> result = new ArrayList<>();
List<IndexIterationPointer> result = new ArrayList<>();
createFromIndexFilterInt(indexFilter, compositeIndex, descending, evalContext, result);
result = IndexIterationPointer.normalizePointers(result, descending);
return result.toArray(new IndexIterationPointer[0]);
}

Expand All @@ -140,13 +141,26 @@ private static void createFromIndexFilterInt(
// based on null end (before conversion) meaning different things.
// The above affects only `from` because NULLs are smaller than any other value and only DESC sort order
// for which `to` is updated during the scan.
result.add(IndexIterationPointer.create(!compositeIndex && descending ? NULL : null, true,
null, true, descending, null));
if (!compositeIndex && descending) {
result.add(IndexIterationPointer.ALL_ALT_DESC);
} else {
result.add(descending ? IndexIterationPointer.ALL_DESC : IndexIterationPointer.ALL);
}
}
if (indexFilter instanceof IndexRangeFilter) {
IndexRangeFilter rangeFilter = (IndexRangeFilter) indexFilter;

Comparable<?> from = null;
if (rangeFilter.getFrom() == null && rangeFilter.getTo() == null) {
// IS NOT NULL range
assert !compositeIndex : "IS NOT NULL range should not be generated for composite index";
result.add(descending ? IndexIterationPointer.IS_NOT_NULL_DESC : IndexIterationPointer.IS_NOT_NULL);
return;
}

// Range filter for non-composite index never includes NULLs.
// Composite index should have both ends specified, and they might cover also NULL values for components
// but from/to will never be NULL but CompositeValue.
Comparable<?> from = compositeIndex ? null : NULL;
if (rangeFilter.getFrom() != null) {
Comparable<?> fromValue = rangeFilter.getFrom().getValue(evalContext);
// If the index filter has expression like a > NULL, we need to
Expand All @@ -168,11 +182,24 @@ private static void createFromIndexFilterInt(
to = toValue;
}

if (from != null && to != null) {
int cmp = ((Comparable) from).compareTo(to);
if (cmp > 0 || (cmp == 0 && (!rangeFilter.isFromInclusive() || !rangeFilter.isToInclusive()))) {
// Range scan with from > to would produce empty result.
// Range scan which reduces to point lookup (from = to) produces result only
// if both ends are inclusive. Otherwise, result would be empty.
// Do not create iteration pointer for such scans (as they would be invalid).
return;
}
}

result.add(IndexIterationPointer.create(
from, rangeFilter.isFromInclusive(), to, rangeFilter.isToInclusive(), descending, null));
} else if (indexFilter instanceof IndexEqualsFilter) {
IndexEqualsFilter equalsFilter = (IndexEqualsFilter) indexFilter;
Comparable<?> value = equalsFilter.getComparable(evalContext);
// Note: this branch is also used for IS NULL, but null value in IndexEqualsFilter
// is mapped to NULL by getComparable, so we can easily use it in the same way as ordinary values.
result.add(IndexIterationPointer.create(value, true, value, true, descending, null));
} else if (indexFilter instanceof IndexCompositeFilter) {
IndexCompositeFilter inFilter = (IndexCompositeFilter) indexFilter;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,22 @@ private static IndexComponentFilter convertFromRangeFilters(
boolean fromInclusive = false;
IndexFilterValue to = null;
boolean toInclusive = false;
boolean found = false;
List<RexNode> expressions = new ArrayList<>(2);

for (IndexComponentCandidate candidate : candidates) {
if (!(candidate.getFilter() instanceof IndexRangeFilter)) {
continue;
}

found = true;
IndexRangeFilter candidateFilter = (IndexRangeFilter) candidate.getFilter();

// Use first matching candidate to define range.
// We do not expect many candidates with literal values as they should be simplified by Calcite.
// When there are both literals and dynamic parameters, we choose one of them.
// Maybe we could be preferring literals, but in general it is not possible to know upfront
// which one would be better.
if (from == null && candidateFilter.getFrom() != null) {
from = candidateFilter.getFrom();
fromInclusive = candidateFilter.isFromInclusive();
Expand All @@ -142,9 +149,14 @@ private static IndexComponentFilter convertFromRangeFilters(
toInclusive = candidateFilter.isToInclusive();
expressions.add(candidate.getExpression());
}

if (from == null && to == null && candidateFilter.getFrom() == null && candidateFilter.getTo() == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR, but feels like all these specific condition checks to handle NULL should live somewhere outside of this class or alternatively be moved to their own separate methods to convey logic more clearly, although maybe comments would work too.

Copy link
Contributor Author

@k-jamroz k-jamroz Oct 10, 2023

Choose a reason for hiding this comment

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

the null/NULL checks generally feel very inconsistent, especially in different layers (optimizer, DAG, Index and IndexStore, Predicate API has also its own logic). I had lot's of headaches because of that but I did not feel confident enough to make it more consistent. Sometimes null means NULL, sometimes unbounded (including NULL or not), sometimes it is invalid... (probably somewhere is also means NOT NULL - for range filters)

// IS NOT NULL filter
expressions.add(candidate.getExpression());
}
}

if (from != null || to != null) {
if (found) {
IndexRangeFilter filter = new IndexRangeFilter(from, fromInclusive, to, toInclusive);
return new IndexComponentFilter(filter, expressions, converterType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ private static Map<Integer, List<IndexComponentCandidate>> prepareSingleColumnCa
* @param exp expression
* @return candidate or {@code null} if the expression cannot be used by any index implementation
*/
@SuppressWarnings("checkstyle:CyclomaticComplexity")
@SuppressWarnings({"checkstyle:CyclomaticComplexity", "checkstyle:ReturnCount"})
private static IndexComponentCandidate prepareSingleColumnCandidate(
RexNode exp,
QueryParameterMetadata parameterMetadata
Expand Down Expand Up @@ -400,6 +400,13 @@ private static IndexComponentCandidate prepareSingleColumnCandidate(
removeCastIfPossible(((RexCall) exp).getOperands().get(0))
);

case IS_NOT_NULL:
// Handle SELECT * FROM WHERE column IS NOT NULL.
return prepareSingleColumnCandidateIsNotNull(
exp,
removeCastIfPossible(((RexCall) exp).getOperands().get(0))
);

case GREATER_THAN:
case GREATER_THAN_OR_EQUAL:
case LESS_THAN:
Expand Down Expand Up @@ -554,6 +561,33 @@ private static IndexComponentCandidate prepareSingleColumnCandidateIsNull(RexNod
);
}

/**
* Try creating a candidate filter for the "IS NOT NULL" expression.
* <p>
* Returns the filter RANGE(-inf..+inf) with "allowNulls-false".
*
* @param exp original expression, e.g. {col IS NOT NULL}
* @param operand operand, e.g. {col}; CAST must be unwrapped before the method is invoked
* @return candidate or {@code null}
*/
private static IndexComponentCandidate prepareSingleColumnCandidateIsNotNull(RexNode exp, RexNode operand) {
if (operand.getKind() != SqlKind.INPUT_REF) {
// The operand is not a column, e.g. {'literal' IS NOT NULL}, index cannot be used
return null;
}

int columnIndex = ((RexInputRef) operand).getIndex();

// Create a range scan for entire range (-inf..+inf), range scan does not include nulls
IndexFilter filter = new IndexRangeFilter(null, false, null, false);

return new IndexComponentCandidate(
exp,
columnIndex,
filter
);
}

/**
* Try creating a candidate filter for comparison operator.
*
Expand Down Expand Up @@ -648,7 +682,7 @@ private static IndexComponentCandidate prepareSingleColumnCandidateComparison(
);
}

@SuppressWarnings({"ConstantConditions", "UnstableApiUsage"})
@SuppressWarnings({"ConstantConditions"})
private static IndexComponentCandidate prepareSingleColumnSearchCandidateComparison(
RexNode exp,
RexNode operand1,
Expand Down Expand Up @@ -783,11 +817,6 @@ private static IndexComponentCandidate prepareSingleColumnCandidateOr(

IndexFilter candidateFilter = candidate.getFilter();

if (!(candidateFilter instanceof IndexEqualsFilter || candidateFilter instanceof IndexCompositeFilter)) {
// Support only equality for ORs
return null;
}

// Make sure that all '=' expressions relate to a single column
if (columnIndex == null) {
columnIndex = candidate.getColumnIndex();
Expand All @@ -796,7 +825,7 @@ private static IndexComponentCandidate prepareSingleColumnCandidateOr(
}

// Flatten. E.g. ((a=1 OR a=2) OR a=3) is parsed into IN(1, 2) and OR(3), that is then flatten into IN(1, 2, 3)
if (candidateFilter instanceof IndexEqualsFilter) {
if (candidateFilter instanceof IndexEqualsFilter || candidateFilter instanceof IndexRangeFilter) {
filters.add(candidateFilter);
} else {
filters.addAll(((IndexCompositeFilter) candidateFilter).getFilters());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
import java.util.Objects;

/**
* Filter the is used for range requests. Could have either lower bound, upper bound or both.
* Filter the is used for range requests. Could have either lower bound, upper bound, both
* or none ({@code IS NOT NULL}).
* <p>
* For non-composite index: matches only NOT NULL values. If any of the bounds
* is {@link com.hazelcast.query.impl.AbstractIndex#NULL}, matches nothing.
Expand Down Expand Up @@ -61,7 +62,6 @@ public IndexRangeFilter() {
}

public IndexRangeFilter(IndexFilterValue from, boolean fromInclusive, IndexFilterValue to, boolean toInclusive) {
assert from != null || to != null;
assert from != null || !fromInclusive : "Unspecified from end must not be inclusive";
assert to != null || !toInclusive : "Unspecified to end must not be inclusive";

Expand Down