Skip to content

Commit

Permalink
Code review updates
Browse files Browse the repository at this point in the history
  • Loading branch information
craigtaverner committed Nov 30, 2023
1 parent 532838e commit 9a86c58
Show file tree
Hide file tree
Showing 17 changed files with 63 additions and 63 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1180,10 +1180,16 @@ public static String randomDateFormatterPattern() {
return randomFrom(FormatNames.values()).getName();
}

/**
* Generate a random valid point constrained to geographic ranges (lat, lon ranges).
*/
public static SpatialPoint randomGeoPoint() {
return new GeoPoint(randomDoubleBetween(-90, 90, true), randomDoubleBetween(-180, 180, true));
}

/**
* Generate a random valid point constrained to cartesian ranges.
*/
public static SpatialPoint randomCartesianPoint() {
double x = randomDoubleBetween(-Float.MAX_VALUE, Float.MAX_VALUE, true);
double y = randomDoubleBetween(-Float.MAX_VALUE, Float.MAX_VALUE, true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
import static org.elasticsearch.xpack.esql.CsvTestUtils.logMetaData;
import static org.elasticsearch.xpack.ql.util.DateUtils.UTC_DATE_TIME_FORMATTER;
import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsNumber;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;
import static org.hamcrest.Matchers.instanceOf;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertThat;
Expand Down Expand Up @@ -202,9 +202,9 @@ public static void assertData(
if (expectedType == Type.DATETIME) {
expectedValue = rebuildExpected(expectedValue, Long.class, x -> UTC_DATE_TIME_FORMATTER.formatMillis((long) x));
} else if (expectedType == Type.GEO_POINT) {
expectedValue = rebuildExpected(expectedValue, Long.class, x -> Geo.longAsPoint((long) x));
expectedValue = rebuildExpected(expectedValue, Long.class, x -> GEO.longAsPoint((long) x));
} else if (expectedType == Type.CARTESIAN_POINT) {
expectedValue = rebuildExpected(expectedValue, Long.class, x -> Cartesian.longAsPoint((long) x));
expectedValue = rebuildExpected(expectedValue, Long.class, x -> CARTESIAN.longAsPoint((long) x));
} else if (expectedType == Type.IP) {
// convert BytesRef-packed IP to String, allowing subsequent comparison with what's expected
expectedValue = rebuildExpected(expectedValue, BytesRef.class, x -> DocValueFormat.IP.format((BytesRef) x));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@
import static org.elasticsearch.xpack.ql.type.DataTypeConverter.safeToUnsignedLong;
import static org.elasticsearch.xpack.ql.util.DateUtils.UTC_DATE_TIME_FORMATTER;
import static org.elasticsearch.xpack.ql.util.NumericUtils.asLongUnsigned;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;

public final class CsvTestUtils {
private static final int MAX_WIDTH = 20;
Expand Down Expand Up @@ -389,8 +389,8 @@ public enum Type {
Long.class
),
BOOLEAN(Booleans::parseBoolean, Boolean.class),
GEO_POINT(x -> x == null ? null : Geo.pointAsLong(Geo.stringAsPoint(x)), Long.class),
CARTESIAN_POINT(x -> x == null ? null : Cartesian.pointAsLong(Cartesian.stringAsPoint(x)), Long.class);
GEO_POINT(x -> x == null ? null : GEO.pointAsLong(GEO.stringAsPoint(x)), Long.class),
CARTESIAN_POINT(x -> x == null ? null : CARTESIAN.pointAsLong(CARTESIAN.stringAsPoint(x)), Long.class);

private static final Map<String, Type> LOOKUP = new HashMap<>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@
import org.elasticsearch.compute.data.BytesRefBlock;
import org.elasticsearch.compute.data.DoubleBlock;
import org.elasticsearch.compute.data.IntBlock;
import org.elasticsearch.compute.data.LongArrayBlock;
import org.elasticsearch.compute.data.LongBlock;
import org.elasticsearch.compute.data.LongVectorBlock;
import org.elasticsearch.compute.lucene.UnsupportedValueSource;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.xcontent.InstantiatingObjectParser;
Expand All @@ -37,8 +35,8 @@
import static org.elasticsearch.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.xpack.ql.util.DateUtils.UTC_DATE_TIME_FORMATTER;
import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsNumber;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;

public record ColumnInfo(String name, String type) implements Writeable {

Expand Down Expand Up @@ -169,10 +167,8 @@ protected XContentBuilder valueToXContent(XContentBuilder builder, ToXContent.Pa
protected XContentBuilder valueToXContent(XContentBuilder builder, ToXContent.Params params, int valueIndex)
throws IOException {
// TODO Perhaps this is just a long for geo_point? And for more advanced types we need a new block type
long encoded = (block instanceof LongVectorBlock)
? ((LongVectorBlock) block).getLong(valueIndex) // needed for single-value fields
: ((LongArrayBlock) block).getLong(valueIndex); // needed when using multi-value fields
String wkt = Geo.pointAsString(Geo.longAsPoint(encoded));
long encoded = ((LongBlock) block).getLong(valueIndex);
String wkt = GEO.pointAsString(GEO.longAsPoint(encoded));
return builder.value(wkt);
}
};
Expand All @@ -181,10 +177,8 @@ protected XContentBuilder valueToXContent(XContentBuilder builder, ToXContent.Pa
protected XContentBuilder valueToXContent(XContentBuilder builder, ToXContent.Params params, int valueIndex)
throws IOException {
// TODO Perhaps this is just a long for cartesian_point? And for more advanced types we need a new block type
long encoded = (block instanceof LongVectorBlock)
? ((LongVectorBlock) block).getLong(valueIndex) // needed for single-value fields
: ((LongArrayBlock) block).getLong(valueIndex); // needed when using multi-value fields
String wkt = Cartesian.pointAsString(Cartesian.longAsPoint(encoded));
long encoded = ((LongBlock) block).getLong(valueIndex);
String wkt = CARTESIAN.pointAsString(CARTESIAN.longAsPoint(encoded));
return builder.value(wkt);
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@
import static org.elasticsearch.xpack.ql.util.DateUtils.UTC_DATE_TIME_FORMATTER;
import static org.elasticsearch.xpack.ql.util.NumericUtils.asLongUnsigned;
import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsNumber;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;
import static org.elasticsearch.xpack.ql.util.StringUtils.parseIP;

public class EsqlQueryResponse extends ActionResponse implements ChunkedToXContent, Releasable {
Expand Down Expand Up @@ -263,8 +263,8 @@ private static Object valueAt(String dataType, Block block, int offset, BytesRef
}
case "boolean" -> ((BooleanBlock) block).getBoolean(offset);
case "version" -> new Version(((BytesRefBlock) block).getBytesRef(offset, scratch)).toString();
case "geo_point" -> Geo.longAsPoint(((LongBlock) block).getLong(offset));
case "cartesian_point" -> Cartesian.longAsPoint(((LongBlock) block).getLong(offset));
case "geo_point" -> GEO.longAsPoint(((LongBlock) block).getLong(offset));
case "cartesian_point" -> CARTESIAN.longAsPoint(((LongBlock) block).getLong(offset));
case "unsupported" -> UnsupportedValueSource.UNSUPPORTED_OUTPUT;
case "_source" -> {
BytesRef val = ((BytesRefBlock) block).getBytesRef(offset, scratch);
Expand Down Expand Up @@ -323,11 +323,11 @@ private static Page valuesToPage(List<String> dataTypes, List<List<Object>> valu
}
}
case "geo_point" -> {
long longVal = Geo.pointAsLong(Geo.stringAsPoint(value.toString()));
long longVal = GEO.pointAsLong(GEO.stringAsPoint(value.toString()));
((LongBlock.Builder) builder).appendLong(longVal);
}
case "cartesian_point" -> {
long longVal = Cartesian.pointAsLong(Cartesian.stringAsPoint(value.toString()));
long longVal = CARTESIAN.pointAsLong(CARTESIAN.stringAsPoint(value.toString()));
((LongBlock.Builder) builder).appendLong(longVal);
}
default -> throw EsqlIllegalArgumentException.illegalDataType(dataTypes.get(c));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import static org.elasticsearch.xpack.ql.type.DataTypes.LONG;
import static org.elasticsearch.xpack.ql.type.DataTypes.TEXT;
import static org.elasticsearch.xpack.ql.type.DataTypes.UNSIGNED_LONG;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;

public class ToCartesianPoint extends AbstractConvertFunction {

Expand Down Expand Up @@ -60,6 +60,6 @@ protected NodeInfo<? extends Expression> info() {

@ConvertEvaluator(extraName = "FromString", warnExceptions = { IllegalArgumentException.class })
static long fromKeyword(BytesRef in) {
return Cartesian.pointAsLong(Cartesian.stringAsPoint(in.utf8ToString()));
return CARTESIAN.pointAsLong(CARTESIAN.stringAsPoint(in.utf8ToString()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import static org.elasticsearch.xpack.ql.type.DataTypes.LONG;
import static org.elasticsearch.xpack.ql.type.DataTypes.TEXT;
import static org.elasticsearch.xpack.ql.type.DataTypes.UNSIGNED_LONG;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;

public class ToGeoPoint extends AbstractConvertFunction {

Expand Down Expand Up @@ -60,6 +60,6 @@ protected NodeInfo<? extends Expression> info() {

@ConvertEvaluator(extraName = "FromString", warnExceptions = { IllegalArgumentException.class })
static long fromKeyword(BytesRef in) {
return Geo.pointAsLong(Geo.stringAsPoint(in.utf8ToString()));
return GEO.pointAsLong(GEO.stringAsPoint(in.utf8ToString()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
import static org.elasticsearch.xpack.ql.type.DataTypes.VERSION;
import static org.elasticsearch.xpack.ql.util.DateUtils.UTC_DATE_TIME_FORMATTER;
import static org.elasticsearch.xpack.ql.util.NumericUtils.unsignedLongAsNumber;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;

public class ToString extends AbstractConvertFunction implements EvaluatorMapper {

Expand Down Expand Up @@ -139,11 +139,11 @@ static BytesRef fromUnsignedLong(long lng) {

@ConvertEvaluator(extraName = "FromGeoPoint")
static BytesRef fromGeoPoint(long point) {
return new BytesRef(Geo.pointAsString(Geo.longAsPoint(point)));
return new BytesRef(GEO.pointAsString(GEO.longAsPoint(point)));
}

@ConvertEvaluator(extraName = "FromCartesianPoint")
static BytesRef fromCartesianPoint(long point) {
return new BytesRef(Cartesian.pointAsString(Cartesian.longAsPoint(point)));
return new BytesRef(CARTESIAN.pointAsString(CARTESIAN.longAsPoint(point)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@
import java.util.ArrayList;
import java.util.List;

import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;
import static org.hamcrest.Matchers.equalTo;

public class EsqlQueryResponseTests extends AbstractChunkedSerializingTestCase<EsqlQueryResponse> {
Expand Down Expand Up @@ -117,8 +117,8 @@ private Page randomPage(List<ColumnInfo> columns) {
new BytesRef(UnsupportedValueSource.UNSUPPORTED_OUTPUT)
);
case "version" -> ((BytesRefBlock.Builder) builder).appendBytesRef(new Version(randomIdentifier()).toBytesRef());
case "geo_point" -> ((LongBlock.Builder) builder).appendLong(Geo.pointAsLong(randomGeoPoint()));
case "cartesian_point" -> ((LongBlock.Builder) builder).appendLong(Cartesian.pointAsLong(randomCartesianPoint()));
case "geo_point" -> ((LongBlock.Builder) builder).appendLong(GEO.pointAsLong(randomGeoPoint()));
case "cartesian_point" -> ((LongBlock.Builder) builder).appendLong(CARTESIAN.pointAsLong(randomCartesianPoint()));
case "null" -> builder.appendNull();
case "_source" -> {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@
import static org.elasticsearch.compute.data.BlockUtils.toJavaObject;
import static org.elasticsearch.xpack.esql.SerializationTestUtils.assertSerialization;
import static org.elasticsearch.xpack.esql.type.EsqlDataTypes.isSpatial;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;
import static org.hamcrest.Matchers.either;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.not;
Expand Down Expand Up @@ -120,8 +120,8 @@ public static Literal randomLiteral(DataType type) {
case "time_duration" -> Duration.ofMillis(randomLongBetween(-604800000L, 604800000L)); // plus/minus 7 days
case "text" -> new BytesRef(randomAlphaOfLength(50));
case "version" -> randomVersion().toBytesRef();
case "geo_point" -> Geo.pointAsLong(randomGeoPoint());
case "cartesian_point" -> Cartesian.pointAsLong(randomCartesianPoint());
case "geo_point" -> GEO.pointAsLong(randomGeoPoint());
case "cartesian_point" -> CARTESIAN.pointAsLong(randomCartesianPoint());
case "null" -> null;
case "_source" -> {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@

import static org.elasticsearch.test.ESTestCase.randomCartesianPoint;
import static org.elasticsearch.test.ESTestCase.randomGeoPoint;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;
import static org.hamcrest.Matchers.equalTo;

/**
Expand Down Expand Up @@ -680,12 +680,12 @@ private static List<TypedDataSupplier> dateCases() {
}

private static List<TypedDataSupplier> geoPointCases() {
return List.of(new TypedDataSupplier("<geo_point>", () -> Geo.pointAsLong(randomGeoPoint()), EsqlDataTypes.GEO_POINT));
return List.of(new TypedDataSupplier("<geo_point>", () -> GEO.pointAsLong(randomGeoPoint()), EsqlDataTypes.GEO_POINT));
}

private static List<TypedDataSupplier> cartesianPointCases() {
return List.of(
new TypedDataSupplier("<cartesian_point>", () -> Cartesian.pointAsLong(randomCartesianPoint()), EsqlDataTypes.CARTESIAN_POINT)
new TypedDataSupplier("<cartesian_point>", () -> CARTESIAN.pointAsLong(randomCartesianPoint()), EsqlDataTypes.CARTESIAN_POINT)
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@
import java.util.List;
import java.util.function.Supplier;

import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;

public class ToStringTests extends AbstractFunctionTestCase {
public ToStringTests(@Name("TestCase") Supplier<TestCaseSupplier.TestCase> testCaseSupplier) {
Expand Down Expand Up @@ -91,14 +91,14 @@ public static Iterable<Object[]> parameters() {
suppliers,
"ToStringFromGeoPointEvaluator[field=" + read + "]",
DataTypes.KEYWORD,
i -> new BytesRef(Geo.pointAsString(Geo.longAsPoint(i))),
i -> new BytesRef(GEO.pointAsString(GEO.longAsPoint(i))),
List.of()
);
TestCaseSupplier.forUnaryCartesianPoint(
suppliers,
"ToStringFromCartesianPointEvaluator[field=" + read + "]",
DataTypes.KEYWORD,
i -> new BytesRef(Cartesian.pointAsString(Cartesian.longAsPoint(i))),
i -> new BytesRef(CARTESIAN.pointAsString(CARTESIAN.longAsPoint(i))),
List.of()
);
TestCaseSupplier.forUnaryIp(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
import java.util.stream.LongStream;
import java.util.stream.Stream;

import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Cartesian;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.CARTESIAN;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;

public abstract class AbstractMultivalueFunctionTestCase extends AbstractScalarFunctionTestCase {
/**
Expand Down Expand Up @@ -411,7 +411,7 @@ protected static void geoPoints(
DataType expectedDataType,
BiFunction<Integer, LongStream, Matcher<Object>> matcher
) {
points(cases, name, evaluatorName, EsqlDataTypes.GEO_POINT, expectedDataType, Geo, ESTestCase::randomGeoPoint, matcher);
points(cases, name, evaluatorName, EsqlDataTypes.GEO_POINT, expectedDataType, GEO, ESTestCase::randomGeoPoint, matcher);
}

/**
Expand Down Expand Up @@ -443,7 +443,7 @@ protected static void cartesianPoints(
evaluatorName,
EsqlDataTypes.CARTESIAN_POINT,
expectedDataType,
Cartesian,
CARTESIAN,
ESTestCase::randomCartesianPoint,
matcher
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import static org.elasticsearch.xpack.esql.formatter.TextFormat.CSV;
import static org.elasticsearch.xpack.esql.formatter.TextFormat.PLAIN_TEXT;
import static org.elasticsearch.xpack.esql.formatter.TextFormat.TSV;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.Geo;
import static org.elasticsearch.xpack.ql.util.SpatialCoordinateTypes.GEO;

public class TextFormatTests extends ESTestCase {

Expand Down Expand Up @@ -255,7 +255,7 @@ private static EsqlQueryResponse regularData() {
.appendBytesRef(new BytesRef("Mind Train"))
.build(),
new IntArrayVector(new int[] { 11 * 60 + 48, 4 * 60 + 40 }, 2).asBlock(),
new LongArrayVector(new long[] { Geo.pointAsLong(12, 56), Geo.pointAsLong(-97, 26) }, 2).asBlock()
new LongArrayVector(new long[] { GEO.pointAsLong(12, 56), GEO.pointAsLong(-97, 26) }, 2).asBlock()
)
);

Expand Down
Loading

0 comments on commit 9a86c58

Please sign in to comment.