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.

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

Jump to bottom

Fix handling of query conditions in delete/update with WHERE doc-keys #15293

Merged
merged 3 commits into from Jan 8, 2024
Merged

Fix handling of query conditions in delete/update with WHERE doc-keys #15293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
acd7d20
Fix handling of query conditions in delete/update with WHERE doc-keys
mfussenegger Jan 8, 2024
e6810a9
Merge branch 'master' into j/update-by-id
mfussenegger Jan 8, 2024
171dfd2
Merge branch 'master' into j/update-by-id
mergify[bot] Jan 8, 2024
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
5 changes: 5 additions & 0 deletions docs/appendices/release-notes/5.5.3.rst
View file
Open in desktop
Expand Up @@ -49,6 +49,11 @@ See the :ref:`version_5.5.0` release notes for a full list of changes in the
Fixes
=====

- Fixed an issue that caused ``UPDATE`` and ``DELETE`` statements to match
records if the ``WHERE`` clause contained an equality condition on all primary
keys, and it included an additional clause in an ``AND`` that should have
evaluated to ``FALSE``.

- Fixed a regression introduced in 5.3.0 that caused storing an object as
``NULL`` if object had generated sub-columns and the object column wasn't
part of the ``INSERT`` targets. An object with generated sub-columns is
Expand Down
73 changes: 35 additions & 38 deletions server/src/main/java/io/crate/analyze/where/EqualityExtractor.java
View file
Open in desktop
Expand Up @@ -26,18 +26,18 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

import org.jetbrains.annotations.Nullable;
import java.util.Set;

import org.elasticsearch.common.io.stream.StreamOutput;
import org.jetbrains.annotations.Nullable;

import io.crate.common.collections.CartesianList;
import io.crate.expression.eval.EvaluatingNormalizer;
import io.crate.expression.operator.AndOperator;
import io.crate.expression.operator.EqOperator;
import io.crate.expression.operator.Operator;
import io.crate.expression.operator.Operators;
Expand Down Expand Up @@ -73,13 +73,17 @@ public EqualityExtractor(EvaluatingNormalizer normalizer) {
this.normalizer = normalizer;
}

public List<List<Symbol>> extractParentMatches(List<ColumnIdent> columns, Symbol symbol, @Nullable TransactionContext coordinatorTxnCtx) {
public record EqMatches(@Nullable List<List<Symbol>> matches, Set<Symbol> unknowns) {

public static final EqMatches NONE = new EqMatches(null, Set.of());
}

public EqMatches extractParentMatches(List<ColumnIdent> columns, Symbol symbol, @Nullable TransactionContext coordinatorTxnCtx) {
return extractMatches(columns, symbol, false, coordinatorTxnCtx);
}

/**
* @return Exact matches of the {@code columns} within {@code symbol}.
* Null if there are no exact matches
* @return Matches of the {@code columns} within {@code symbol}.
*
* <pre>
* Example for exact matches:
Expand All @@ -101,25 +105,25 @@ public List<List<Symbol>> extractParentMatches(List<ColumnIdent> columns, Symbol
* x = 10 and match(z, ...) = 'foo' -> null (MATCH predicate can only be applied on lucene)
* </pre>
*/
@Nullable
public List<List<Symbol>> extractExactMatches(List<ColumnIdent> columns,
Symbol symbol,
@Nullable TransactionContext transactionContext) {
public EqMatches extractMatches(List<ColumnIdent> columns,
Symbol symbol,
@Nullable TransactionContext transactionContext) {
return extractMatches(columns, symbol, true, transactionContext);
}

@Nullable
private List<List<Symbol>> extractMatches(Collection<ColumnIdent> columns,
Symbol symbol,
boolean exact,
@Nullable TransactionContext transactionContext) {
EqualityExtractor.ProxyInjectingVisitor.Context context =
new EqualityExtractor.ProxyInjectingVisitor.Context(columns, exact);
private EqMatches extractMatches(Collection<ColumnIdent> columns,
Symbol symbol,
boolean shortCircuitOnMatchPredicateUnknown,
@Nullable TransactionContext transactionContext) {
var context = new ProxyInjectingVisitor.Context(columns);
Symbol proxiedTree = symbol.accept(ProxyInjectingVisitor.INSTANCE, context);

// bail out if we have any unknown part in the tree
if (context.exact && context.seenUnknown) {
return null;
// Match cannot execute without Lucene
if (shortCircuitOnMatchPredicateUnknown && context.unknowns.size() == 1) {
Symbol unknown = context.unknowns.iterator().next();
if (unknown instanceof MatchPredicate || unknown instanceof Function fn && fn.name().equals(io.crate.expression.predicate.MatchPredicate.NAME)) {
return EqMatches.NONE;
}
}

List<List<EqProxy>> comparisons = context.comparisonValues();
Expand All @@ -138,7 +142,7 @@ private List<List<Symbol>> extractMatches(Collection<ColumnIdent> columns,
Symbol normalized = normalizer.normalize(proxiedTree, transactionContext);
if (normalized == Literal.BOOLEAN_TRUE) {
if (anyNull) {
return null;
return EqMatches.NONE;
}
if (!proxies.isEmpty()) {
List<Symbol> row = new ArrayList<>(proxies.size());
Expand All @@ -154,8 +158,7 @@ private List<List<Symbol>> extractMatches(Collection<ColumnIdent> columns,
}
}
}
return result.isEmpty() ? null : result;

return new EqMatches(result.isEmpty() ? null : result, context.unknowns);
}

/**
Expand Down Expand Up @@ -367,12 +370,9 @@ static class Context {

private LinkedHashMap<ColumnIdent, Comparison> comparisons;
private boolean proxyBelow;
private boolean seenUnknown = false;
private boolean ignoreUnknown = false;
private final boolean exact;
private final Set<Symbol> unknowns = new HashSet<>();

private Context(Collection<ColumnIdent> references, boolean exact) {
this.exact = exact;
private Context(Collection<ColumnIdent> references) {
comparisons = new LinkedHashMap<>(references.size());
for (ColumnIdent reference : references) {
comparisons.put(reference, new Comparison());
Expand All @@ -395,16 +395,16 @@ protected Symbol visitSymbol(Symbol symbol, Context context) {

@Override
public Symbol visitMatchPredicate(MatchPredicate matchPredicate, Context context) {
context.seenUnknown = true;
context.unknowns.add(matchPredicate);
return Literal.BOOLEAN_TRUE;
}

@Override
public Symbol visitReference(Reference symbol, Context context) {
if (!context.comparisons.containsKey(symbol.column())) {
context.seenUnknown = true;
public Symbol visitReference(Reference ref, Context context) {
if (!context.comparisons.containsKey(ref.column())) {
context.unknowns.add(ref);
}
return super.visitReference(symbol, context);
return super.visitReference(ref, context);
}

public Symbol visitFunction(Function function, Context context) {
Expand Down Expand Up @@ -435,7 +435,6 @@ public Symbol visitFunction(Function function, Context context) {
} else if (Operators.LOGICAL_OPERATORS.contains(functionName)) {
boolean proxyBelowPre = context.proxyBelow;
boolean proxyBelowPost = proxyBelowPre;
context.ignoreUnknown = context.ignoreUnknown || functionName.equals(AndOperator.NAME);
ArrayList<Symbol> newArgs = new ArrayList<>(arguments.size());
for (Symbol arg : arguments) {
context.proxyBelow = proxyBelowPre;
Expand All @@ -448,10 +447,8 @@ public Symbol visitFunction(Function function, Context context) {
}
return new Function(function.signature(), newArgs, function.valueType());
}
if (context.ignoreUnknown == false
|| functionName.equals(io.crate.expression.predicate.MatchPredicate.NAME)) {
context.seenUnknown = true;
}

context.unknowns.add(function);
return Literal.BOOLEAN_TRUE;
}
}
Expand Down
63 changes: 26 additions & 37 deletions server/src/main/java/io/crate/planner/WhereClauseOptimizer.java
View file
Open in desktop
Expand Up @@ -33,12 +33,12 @@
import io.crate.analyze.WhereClause;
import io.crate.analyze.where.DocKeys;
import io.crate.analyze.where.EqualityExtractor;
import io.crate.analyze.where.EqualityExtractor.EqMatches;
import io.crate.analyze.where.WhereClauseAnalyzer;
import io.crate.analyze.where.WhereClauseValidator;
import io.crate.common.collections.Lists2;
import io.crate.data.Row;
import io.crate.expression.eval.EvaluatingNormalizer;
import io.crate.expression.symbol.RefVisitor;
import io.crate.expression.symbol.Symbol;
import io.crate.expression.symbol.Symbols;
import io.crate.metadata.ColumnIdent;
Expand Down Expand Up @@ -76,12 +76,13 @@ public static class DetailedQuery {
DocKeys docKeys,
List<List<Symbol>> partitionValues,
Set<Symbol> clusteredByValues,
DocTableInfo table) {
DocTableInfo table,
boolean queryHasPkSymbolsOnly) {
this.query = query;
this.docKeys = docKeys;
this.partitions = Objects.requireNonNullElse(partitionValues, Collections.emptyList());
this.clusteredByValues = clusteredByValues;
this.queryHasPkSymbolsOnly = WhereClauseOptimizer.queryHasPkSymbolsOnly(query, table);
this.queryHasPkSymbolsOnly = queryHasPkSymbolsOnly;
}

public Optional<DocKeys> docKeys() {
Expand Down Expand Up @@ -164,19 +165,16 @@ public static DetailedQuery optimize(EvaluatingNormalizer normalizer,
List<ColumnIdent> pkCols = pkColsInclVersioning(table, versionInQuery, sequenceVersioningInQuery);

EqualityExtractor eqExtractor = new EqualityExtractor(normalizer);
List<List<Symbol>> pkValues = eqExtractor.extractExactMatches(pkCols, query, txnCtx);

List<List<Symbol>> partitionValues = null;
if (table.isPartitioned()) {
partitionValues = eqExtractor.extractExactMatches(table.partitionedBy(), query, txnCtx);
}
EqMatches pkMatches = eqExtractor.extractMatches(pkCols, query, txnCtx);
Set<Symbol> clusteredBy = Collections.emptySet();
if (table.clusteredBy() != null) {
List<List<Symbol>> clusteredByValues = eqExtractor.extractParentMatches(
EqualityExtractor.EqMatches clusteredByMatches = eqExtractor.extractParentMatches(
Collections.singletonList(table.clusteredBy()), query, txnCtx);
if (clusteredByValues != null) {
clusteredBy = new HashSet<>(clusteredByValues.size());
for (List<Symbol> s : clusteredByValues) {
List<List<Symbol>> clusteredBySymbols = clusteredByMatches.matches();
if (clusteredBySymbols != null) {
clusteredBy = new HashSet<>(clusteredBySymbols.size());
for (List<Symbol> s : clusteredBySymbols) {
clusteredBy.add(s.get(0));
}
}
Expand All @@ -186,26 +184,37 @@ public static DetailedQuery optimize(EvaluatingNormalizer normalizer,
final boolean shouldUseDocKeys = table.isPartitioned() == false && (
DocSysColumns.ID.equals(table.clusteredBy()) || (
table.primaryKey().size() == 1 && table.clusteredBy().equals(table.primaryKey().get(0))));
if (pkValues == null && shouldUseDocKeys) {
pkValues = eqExtractor.extractExactMatches(List.of(DocSysColumns.ID), query, txnCtx);

if (pkMatches.matches() == null && shouldUseDocKeys) {
pkMatches = eqExtractor.extractMatches(List.of(DocSysColumns.ID), query, txnCtx);
}

if (pkValues == null) {
if (pkMatches.matches() == null) {
docKeys = null;
} else {
List<Integer> partitionIndicesWithinPks = null;
if (table.isPartitioned()) {
partitionIndicesWithinPks = getPartitionIndices(table.primaryKey(), table.partitionedBy());
}
docKeys = new DocKeys(pkValues,
docKeys = new DocKeys(pkMatches.matches(),
versionInQuery,
sequenceVersioningInQuery,
clusterIdxWithinPK,
partitionIndicesWithinPks);
}
EqMatches partitionMatches = table.isPartitioned()
? eqExtractor.extractMatches(table.partitionedBy(), query, txnCtx)
: EqMatches.NONE;

WhereClauseValidator.validate(query);
return new DetailedQuery(query, docKeys, partitionValues, clusteredBy, table);
return new DetailedQuery(
query,
docKeys,
partitionMatches.matches(),
clusteredBy,
table,
pkMatches.unknowns().isEmpty()
);
}

private static List<Integer> getPartitionIndices(List<ColumnIdent> pkCols, List<ColumnIdent> partitionCols) {
Expand All @@ -220,26 +229,6 @@ private static List<Integer> getPartitionIndices(List<ColumnIdent> pkCols, List<
return result;
}

private static boolean queryHasPkSymbolsOnly(Symbol query, DocTableInfo table) {
var docKeyColumns = new ArrayList<>(table.primaryKey());
docKeyColumns.addAll(table.partitionedBy());
docKeyColumns.add(table.clusteredBy());
docKeyColumns.add(DocSysColumns.VERSION);
docKeyColumns.add(DocSysColumns.SEQ_NO);
docKeyColumns.add(DocSysColumns.PRIMARY_TERM);

boolean[] hasPkSymbolsOnly = new boolean[]{true};
RefVisitor.visitRefs(
query,
ref -> {
if (docKeyColumns.contains(ref.column()) == false) {
hasPkSymbolsOnly[0] = false;
}
}
);
return hasPkSymbolsOnly[0];
}

private static List<ColumnIdent> pkColsInclVersioning(DocTableInfo table,
boolean versionInQuery,
boolean seqNoAndPrimaryTermInQuery) {
Expand Down
6 changes: 3 additions & 3 deletions server/src/test/java/io/crate/analyze/where/EqualityExtractorTest.java
View file
Open in desktop
Expand Up @@ -21,8 +21,8 @@

package io.crate.analyze.where;

import static io.crate.testing.Asserts.assertThat;
import static io.crate.testing.Asserts.isLiteral;
import static org.assertj.core.api.Assertions.assertThat;

import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -62,7 +62,7 @@ public void prepare() throws Exception {
}

private List<List<Symbol>> analyzeParentX(Symbol query) {
return ee.extractParentMatches(List.of(x), query, coordinatorTxnCtx);
return ee.extractParentMatches(List.of(x), query, coordinatorTxnCtx).matches();
}

private List<List<Symbol>> analyzeExactX(Symbol query) {
Expand All @@ -74,7 +74,7 @@ private List<List<Symbol>> analyzeExactXI(Symbol query) {
}

private List<List<Symbol>> analyzeExact(Symbol query, List<ColumnIdent> primaryKeys) {
return ee.extractExactMatches(primaryKeys, query, coordinatorTxnCtx);
return ee.extractMatches(primaryKeys, query, coordinatorTxnCtx).matches();
}

private Symbol query(String expression) {
Expand Down
10 changes: 10 additions & 0 deletions server/src/test/java/io/crate/planner/UpdatePlannerTest.java
View file
Open in desktop
Expand Up @@ -154,6 +154,16 @@ public void test_update_plan_with_where_clause_involving_pk_and_non_pk() throws
assertThat(update).isExactlyInstanceOf(UpdatePlanner.Update.class);
}

@Test
public void test_update_with_subquery_and_pk_does_not_update_by_id() throws Exception {
Plan update = e.plan(
"update users set name = 'No!' where id = 1 and not exists (select 1 from users where id = 1)");
assertThat(update).isExactlyInstanceOf(MultiPhasePlan.class);

MultiPhasePlan multiPhasePlan = (MultiPhasePlan) update;
assertThat(multiPhasePlan.rootPlan).isExactlyInstanceOf(UpdatePlanner.Update.class);
}

@Test
public void testUpdatePlanWithMultiplePrimaryKeyValuesPartitioned() throws Exception {
Plan update = e.plan("update parted_pks set name='Vogon lyric fan' where " +
Expand Down