Skip to content

Commit

Permalink
[ESQL] Moving argument compatibility checking for Equals (elastic#107546
Browse files Browse the repository at this point in the history
)

Fixed the conflicts, and re-submitting. Please see elastic#105217 for full details, history, and discussion. I'll use the commit message from that PR as well.

Continuing my work from elastic#104490, this PR moves the parameter compatibility checking for Equals into the type resolution check. This is a somewhat bigger change than for Add, as there was no ES|QL base class for binary comparison operators before this. I've added EsqlBinaryComparison as that base class, and migrated all of the binary comparisons to be based off of that (except for NullEquals, see note below).

In order to maintain compatibility with the current behavior, I've kept it so that unsigned longs are only inter-operable with other unsigned longs. We've talked a lot about changing that, and I consider this work a prerequisite for that.

I've also added a bunch of test cases to Equals and NotEquals, which should have the side effect of filling out the type support table in the equals docs. As noted in the comments, I'll have follow up PRs for the other binary comparisons to add tests, but this PR is already too long.

Note about NullEquals: There is an ES|QL NullEquals class, which inherits from the QL version, but I don't think it works. I didn't see any tests or docs for it, and trying it out in the demo instance gave me a syntax error. I think we need to delve into what's going on there, but this PR isn't the right place for it.

This reverts commit 225edaf.
  • Loading branch information
not-napoleon authored Apr 16, 2024
1 parent e6d421c commit 73a17a1
Show file tree
Hide file tree
Showing 23 changed files with 872 additions and 262 deletions.
5 changes: 0 additions & 5 deletions docs/changelog/107537.yaml

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,9 @@ nullOnMultivaluesComparisonOperation
required_feature: esql.disable_nullable_opts

ROW a = 5, b = [ 1, 2 ]| EVAL same = a == b| LIMIT 1 | WHERE same IS NULL;
warning:Line 1:38: evaluation of [a == b] failed, treating result as null. Only first 20 failures recorded.
warning:Line 1:38: java.lang.IllegalArgumentException: single-value function encountered multi-value


a:integer | b:integer | same:boolean
5 | [1, 2] | null
Expand All @@ -166,6 +169,8 @@ notNullOnMultivaluesComparisonOperation
required_feature: esql.disable_nullable_opts

ROW a = 5, b = [ 1, 2 ]| EVAL same = a == b| LIMIT 1 | WHERE same IS NOT NULL;
warning:Line 1:38: evaluation of [a == b] failed, treating result as null. Only first 20 failures recorded.
warning:Line 1:38: java.lang.IllegalArgumentException: single-value function encountered multi-value

a:integer | b:integer | same:boolean
;
Expand All @@ -175,6 +180,8 @@ notNullOnMultivaluesComparisonOperationWithPartialMatch
required_feature: esql.disable_nullable_opts

ROW a = 5, b = [ 5, 2 ]| EVAL same = a == b| LIMIT 1 | WHERE same IS NOT NULL;
warning:Line 1:38: evaluation of [a == b] failed, treating result as null. Only first 20 failures recorded.
warning:Line 1:38: java.lang.IllegalArgumentException: single-value function encountered multi-value

a:integer | b:integer | same:boolean
;
Original file line number Diff line number Diff line change
Expand Up @@ -8,33 +8,48 @@

import org.apache.lucene.util.BytesRef;
import org.elasticsearch.compute.ann.Evaluator;
import org.elasticsearch.xpack.esql.expression.EsqlTypeResolutions;
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
import org.elasticsearch.xpack.esql.type.EsqlDataTypes;
import org.elasticsearch.xpack.ql.expression.Expression;
import org.elasticsearch.xpack.ql.expression.TypeResolutions;
import org.elasticsearch.xpack.ql.expression.predicate.Negatable;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
import org.elasticsearch.xpack.ql.tree.NodeInfo;
import org.elasticsearch.xpack.ql.tree.Source;
import org.elasticsearch.xpack.ql.type.DataType;
import org.elasticsearch.xpack.ql.type.DataTypes;

import java.time.ZoneId;
import java.util.Map;

public class Equals extends EsqlBinaryComparison implements Negatable<BinaryComparison> {
private static final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap = Map.ofEntries(
Map.entry(DataTypes.BOOLEAN, EqualsBoolsEvaluator.Factory::new),
Map.entry(DataTypes.INTEGER, EqualsIntsEvaluator.Factory::new),
Map.entry(DataTypes.DOUBLE, EqualsDoublesEvaluator.Factory::new),
Map.entry(DataTypes.LONG, EqualsLongsEvaluator.Factory::new),
Map.entry(DataTypes.UNSIGNED_LONG, EqualsLongsEvaluator.Factory::new),
Map.entry(DataTypes.DATETIME, EqualsLongsEvaluator.Factory::new),
Map.entry(EsqlDataTypes.GEO_POINT, EqualsGeometriesEvaluator.Factory::new),
Map.entry(EsqlDataTypes.CARTESIAN_POINT, EqualsGeometriesEvaluator.Factory::new),
Map.entry(EsqlDataTypes.GEO_SHAPE, EqualsGeometriesEvaluator.Factory::new),
Map.entry(EsqlDataTypes.CARTESIAN_SHAPE, EqualsGeometriesEvaluator.Factory::new),
Map.entry(DataTypes.KEYWORD, EqualsKeywordsEvaluator.Factory::new),
Map.entry(DataTypes.TEXT, EqualsKeywordsEvaluator.Factory::new),
Map.entry(DataTypes.VERSION, EqualsKeywordsEvaluator.Factory::new),
Map.entry(DataTypes.IP, EqualsKeywordsEvaluator.Factory::new)
);

import static org.elasticsearch.xpack.ql.expression.TypeResolutions.ParamOrdinal.DEFAULT;

public class Equals extends org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals {
public Equals(Source source, Expression left, Expression right) {
super(source, left, right);
super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.EQ, evaluatorMap);
}

public Equals(Source source, Expression left, Expression right, ZoneId zoneId) {
super(source, left, right, zoneId);
}

@Override
protected TypeResolution resolveInputType(Expression e, TypeResolutions.ParamOrdinal paramOrdinal) {
return EsqlTypeResolutions.isExact(e, sourceText(), DEFAULT);
super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.EQ, zoneId, evaluatorMap);
}

@Override
protected NodeInfo<org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.Equals> info() {
protected NodeInfo<Equals> info() {
return NodeInfo.create(this, Equals::new, left(), right(), zoneId());
}

Expand All @@ -48,6 +63,11 @@ public Equals swapLeftAndRight() {
return new Equals(source(), right(), left(), zoneId());
}

@Override
public BinaryComparison reverse() {
return this;
}

@Override
public BinaryComparison negate() {
return new NotEquals(source(), left(), right(), zoneId());
Expand Down Expand Up @@ -82,4 +102,5 @@ static boolean processBools(boolean lhs, boolean rhs) {
static boolean processGeometries(BytesRef lhs, BytesRef rhs) {
return lhs.equals(rhs);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.esql.evaluator.predicate.operator.comparison;

import org.elasticsearch.compute.operator.EvalOperator;
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
import org.elasticsearch.xpack.esql.evaluator.mapper.EvaluatorMapper;
import org.elasticsearch.xpack.esql.expression.function.scalar.math.Cast;
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
import org.elasticsearch.xpack.esql.type.EsqlDataTypeRegistry;
import org.elasticsearch.xpack.ql.expression.Expression;
import org.elasticsearch.xpack.ql.expression.TypeResolutions;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
import org.elasticsearch.xpack.ql.tree.Source;
import org.elasticsearch.xpack.ql.type.DataType;
import org.elasticsearch.xpack.ql.type.DataTypes;

import java.time.ZoneId;
import java.util.Map;
import java.util.function.Function;

import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
import static org.elasticsearch.xpack.ql.type.DataTypes.UNSIGNED_LONG;

public abstract class EsqlBinaryComparison extends BinaryComparison implements EvaluatorMapper {

private final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap;

protected EsqlBinaryComparison(
Source source,
Expression left,
Expression right,
/* TODO: BinaryComparisonOperator is an enum with a bunch of functionality we don't really want. We should extract an interface and
create a symbol only version like we did for BinaryArithmeticOperation. Ideally, they could be the same class.
*/
BinaryComparisonProcessor.BinaryComparisonOperation operation,
Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap
) {
this(source, left, right, operation, null, evaluatorMap);
}

protected EsqlBinaryComparison(
Source source,
Expression left,
Expression right,
BinaryComparisonProcessor.BinaryComparisonOperation operation,
// TODO: We are definitely not doing the right thing with this zoneId
ZoneId zoneId,
Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap
) {
super(source, left, right, operation, zoneId);
this.evaluatorMap = evaluatorMap;
}

@Override
public EvalOperator.ExpressionEvaluator.Factory toEvaluator(
Function<Expression, EvalOperator.ExpressionEvaluator.Factory> toEvaluator
) {
// Our type is always boolean, so figure out the evaluator type from the inputs
DataType commonType = EsqlDataTypeRegistry.INSTANCE.commonType(left().dataType(), right().dataType());
EvalOperator.ExpressionEvaluator.Factory lhs;
EvalOperator.ExpressionEvaluator.Factory rhs;

if (commonType.isNumeric()) {
lhs = Cast.cast(source(), left().dataType(), commonType, toEvaluator.apply(left()));
rhs = Cast.cast(source(), right().dataType(), commonType, toEvaluator.apply(right()));
} else {
lhs = toEvaluator.apply(left());
rhs = toEvaluator.apply(right());
}

if (evaluatorMap.containsKey(commonType) == false) {
throw new EsqlIllegalArgumentException("Unsupported type " + left().dataType());
}
return evaluatorMap.get(commonType).apply(source(), lhs, rhs);
}

@Override
public Boolean fold() {
return (Boolean) EvaluatorMapper.super.fold();
}

@Override
protected TypeResolution resolveType() {
TypeResolution typeResolution = super.resolveType();
if (typeResolution.unresolved()) {
return typeResolution;
}

return checkCompatibility();
}

@Override
protected TypeResolution resolveInputType(Expression e, TypeResolutions.ParamOrdinal paramOrdinal) {
return TypeResolutions.isType(
e,
evaluatorMap::containsKey,
sourceText(),
paramOrdinal,
evaluatorMap.keySet().stream().map(DataType::typeName).toArray(String[]::new)
);
}

/**
* Check if the two input types are compatible for this operation
*
* @return TypeResolution.TYPE_RESOLVED iff the types are compatible. Otherwise, an appropriate type resolution error.
*/
protected TypeResolution checkCompatibility() {
DataType leftType = left().dataType();
DataType rightType = right().dataType();

// Unsigned long is only interoperable with other unsigned longs
if ((rightType == UNSIGNED_LONG && (false == (leftType == UNSIGNED_LONG || leftType == DataTypes.NULL)))
|| (leftType == UNSIGNED_LONG && (false == (rightType == UNSIGNED_LONG || rightType == DataTypes.NULL)))) {
return new TypeResolution(formatIncompatibleTypesMessage());
}

if ((leftType.isNumeric() && rightType.isNumeric())
|| (DataTypes.isString(leftType) && DataTypes.isString(rightType))
|| leftType.equals(rightType)
|| DataTypes.isNull(leftType)
|| DataTypes.isNull(rightType)) {
return TypeResolution.TYPE_RESOLVED;
}
return new TypeResolution(formatIncompatibleTypesMessage());
}

public String formatIncompatibleTypesMessage() {
if (left().dataType().equals(UNSIGNED_LONG)) {
return format(
null,
"first argument of [{}] is [unsigned_long] and second is [{}]. "
+ "[unsigned_long] can only be operated on together with another [unsigned_long]",
sourceText(),
right().dataType().typeName()
);
}
if (right().dataType().equals(UNSIGNED_LONG)) {
return format(
null,
"first argument of [{}] is [{}] and second is [unsigned_long]. "
+ "[unsigned_long] can only be operated on together with another [unsigned_long]",
sourceText(),
left().dataType().typeName()
);
}
return format(
null,
"first argument of [{}] is [{}] so second argument must also be [{}] but was [{}]",
sourceText(),
left().dataType().isNumeric() ? "numeric" : left().dataType().typeName(),
left().dataType().isNumeric() ? "numeric" : left().dataType().typeName(),
right().dataType().typeName()
);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,42 @@

import org.apache.lucene.util.BytesRef;
import org.elasticsearch.compute.ann.Evaluator;
import org.elasticsearch.xpack.esql.expression.EsqlTypeResolutions;
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
import org.elasticsearch.xpack.ql.expression.Expression;
import org.elasticsearch.xpack.ql.expression.TypeResolutions;
import org.elasticsearch.xpack.ql.expression.predicate.Negatable;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
import org.elasticsearch.xpack.ql.tree.NodeInfo;
import org.elasticsearch.xpack.ql.tree.Source;
import org.elasticsearch.xpack.ql.type.DataType;
import org.elasticsearch.xpack.ql.type.DataTypes;

import java.time.ZoneId;
import java.util.Map;

import static org.elasticsearch.xpack.ql.expression.TypeResolutions.ParamOrdinal.DEFAULT;
public class GreaterThan extends EsqlBinaryComparison implements Negatable<BinaryComparison> {
private static final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap = Map.ofEntries(
Map.entry(DataTypes.INTEGER, GreaterThanIntsEvaluator.Factory::new),
Map.entry(DataTypes.DOUBLE, GreaterThanDoublesEvaluator.Factory::new),
Map.entry(DataTypes.LONG, GreaterThanLongsEvaluator.Factory::new),
Map.entry(DataTypes.UNSIGNED_LONG, GreaterThanLongsEvaluator.Factory::new),
Map.entry(DataTypes.DATETIME, GreaterThanLongsEvaluator.Factory::new),
Map.entry(DataTypes.KEYWORD, GreaterThanKeywordsEvaluator.Factory::new),
Map.entry(DataTypes.TEXT, GreaterThanKeywordsEvaluator.Factory::new),
Map.entry(DataTypes.VERSION, GreaterThanKeywordsEvaluator.Factory::new),
Map.entry(DataTypes.IP, GreaterThanKeywordsEvaluator.Factory::new)
);

public class GreaterThan extends org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThan {
public GreaterThan(Source source, Expression left, Expression right, ZoneId zoneId) {
super(source, left, right, zoneId);
public GreaterThan(Source source, Expression left, Expression right) {
super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.GT, evaluatorMap);
}

@Override
protected TypeResolution resolveInputType(Expression e, TypeResolutions.ParamOrdinal paramOrdinal) {
return EsqlTypeResolutions.isExact(e, sourceText(), DEFAULT);
public GreaterThan(Source source, Expression left, Expression right, ZoneId zoneId) {
super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.GT, zoneId, evaluatorMap);
}

@Override
protected NodeInfo<org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThan> info() {
protected NodeInfo<GreaterThan> info() {
return NodeInfo.create(this, GreaterThan::new, left(), right(), zoneId());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,42 @@

import org.apache.lucene.util.BytesRef;
import org.elasticsearch.compute.ann.Evaluator;
import org.elasticsearch.xpack.esql.expression.EsqlTypeResolutions;
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.EsqlArithmeticOperation;
import org.elasticsearch.xpack.ql.expression.Expression;
import org.elasticsearch.xpack.ql.expression.TypeResolutions;
import org.elasticsearch.xpack.ql.expression.predicate.Negatable;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparison;
import org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.BinaryComparisonProcessor;
import org.elasticsearch.xpack.ql.tree.NodeInfo;
import org.elasticsearch.xpack.ql.tree.Source;
import org.elasticsearch.xpack.ql.type.DataType;
import org.elasticsearch.xpack.ql.type.DataTypes;

import java.time.ZoneId;
import java.util.Map;

import static org.elasticsearch.xpack.ql.expression.TypeResolutions.ParamOrdinal.DEFAULT;
public class GreaterThanOrEqual extends EsqlBinaryComparison implements Negatable<BinaryComparison> {
private static final Map<DataType, EsqlArithmeticOperation.BinaryEvaluator> evaluatorMap = Map.ofEntries(
Map.entry(DataTypes.INTEGER, GreaterThanOrEqualIntsEvaluator.Factory::new),
Map.entry(DataTypes.DOUBLE, GreaterThanOrEqualDoublesEvaluator.Factory::new),
Map.entry(DataTypes.LONG, GreaterThanOrEqualLongsEvaluator.Factory::new),
Map.entry(DataTypes.UNSIGNED_LONG, GreaterThanOrEqualLongsEvaluator.Factory::new),
Map.entry(DataTypes.DATETIME, GreaterThanOrEqualLongsEvaluator.Factory::new),
Map.entry(DataTypes.KEYWORD, GreaterThanOrEqualKeywordsEvaluator.Factory::new),
Map.entry(DataTypes.TEXT, GreaterThanOrEqualKeywordsEvaluator.Factory::new),
Map.entry(DataTypes.VERSION, GreaterThanOrEqualKeywordsEvaluator.Factory::new),
Map.entry(DataTypes.IP, GreaterThanOrEqualKeywordsEvaluator.Factory::new)
);

public class GreaterThanOrEqual extends org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThanOrEqual {

public GreaterThanOrEqual(Source source, Expression left, Expression right, ZoneId zoneId) {
super(source, left, right, zoneId);
public GreaterThanOrEqual(Source source, Expression left, Expression right) {
super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.GTE, evaluatorMap);
}

@Override
protected TypeResolution resolveInputType(Expression e, TypeResolutions.ParamOrdinal paramOrdinal) {
return EsqlTypeResolutions.isExact(e, sourceText(), DEFAULT);
public GreaterThanOrEqual(Source source, Expression left, Expression right, ZoneId zoneId) {
super(source, left, right, BinaryComparisonProcessor.BinaryComparisonOperation.GTE, zoneId, evaluatorMap);
}

@Override
protected NodeInfo<org.elasticsearch.xpack.ql.expression.predicate.operator.comparison.GreaterThanOrEqual> info() {
protected NodeInfo<GreaterThanOrEqual> info() {
return NodeInfo.create(this, GreaterThanOrEqual::new, left(), right(), zoneId());
}

Expand Down
Loading

0 comments on commit 73a17a1

Please sign in to comment.