Skip to content

Commit

Permalink
SQL: Fix esType for DATETIME/DATE and INTERVALS (#38179)
Browse files Browse the repository at this point in the history 
Since introduction of data types that don't have a corresponding type
in ES the `esType` is error-prone when used for `unmappedType()` calls.
Moreover since the renaming of `DATE` to `DATETIME` and the introduction
of an actual date-only `DATE` the `esType` would return `datetime` which
is not a valid type for ES mapping.

Fixes: #38051
  • Loading branch information
@matriv
matriv authored Feb 5, 2019
1 parent 1f4f6f3 commit 2c30501
Show file tree
Hide file tree
Showing 22 changed files with 86 additions and 80 deletions.
4 changes: 2 additions & 2 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ else if (DataTypes.isUnsupported(fa.dataType())) {
// compound fields
else if (allowCompound == false && fa.dataType().isPrimitive() == false) {
named = u.withUnresolvedMessage(
"Cannot use field [" + fa.name() + "] type [" + fa.dataType().esType + "] only its subfields");
"Cannot use field [" + fa.name() + "] type [" + fa.dataType().typeName + "] only its subfields");
}
}
return named;
Expand Down Expand Up @@ -1228,4 +1228,4 @@ protected boolean skipResolved() {
return true;
}
}
}
}
4 changes: 2 additions & 2 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,7 @@ private static void validateInExpression(LogicalPlan p, Set<Failure> localFailur
for (Expression value : in.list()) {
if (areTypesCompatible(dt, value.dataType()) == false) {
localFailures.add(fail(value, "expected data type [{}], value provided is of type [{}]",
dt.esType, value.dataType().esType));
dt.typeName, value.dataType().typeName));
return;
}
}
Expand All @@ -703,7 +703,7 @@ private static void validateConditional(LogicalPlan p, Set<Failure> localFailure
} else {
if (areTypesCompatible(dt, child.dataType()) == false) {
localFailures.add(fail(child, "expected data type [{}], value provided is of type [{}]",
dt.esType, child.dataType().esType));
dt.typeName, child.dataType().typeName));
return;
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/execution/search/SourceGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,4 +175,4 @@ private static void disableSource(SearchSourceBuilder builder) {
builder.storedFields(NO_STORED_FIELD);
}
}
}
}
2 changes: 1 addition & 1 deletion ...c/main/java/org/elasticsearch/xpack/sql/execution/search/extractor/FieldHitExtractor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public String getWriteableName() {
@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeString(fieldName);
out.writeOptionalString(dataType == null ? null : dataType.esType);
out.writeOptionalString(dataType == null ? null : dataType.typeName);
out.writeBoolean(useDocValue);
out.writeOptionalString(hitName);
}
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Expressions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ public static TypeResolution typeMustBe(Expression e,
paramOrd == null || paramOrd == ParamOrdinal.DEFAULT ? "" : " " + paramOrd.name().toLowerCase(Locale.ROOT),
acceptedTypesForErrorMsg(acceptedTypes),
Expressions.name(e),
e.dataType().esType));
e.dataType().typeName));
}

private static String acceptedTypesForErrorMsg(String... acceptedTypes) {
Expand Down
4 changes: 2 additions & 2 deletions .../sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/aggregate/TopHits.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ protected TypeResolution resolveType() {
((FieldAttribute) field()).exactAttribute();
} catch (MappingException ex) {
return new TypeResolution(format(null, "[{}] cannot operate on first argument field of data type [{}]",
functionName(), field().dataType().esType));
functionName(), field().dataType().typeName));
}

if (orderField() != null) {
Expand All @@ -59,7 +59,7 @@ protected TypeResolution resolveType() {
((FieldAttribute) orderField()).exactAttribute();
} catch (MappingException ex) {
return new TypeResolution(format(null, "[{}] cannot operate on second argument field of data type [{}]",
functionName(), orderField().dataType().esType));
functionName(), orderField().dataType().typeName));
}
}
return TypeResolution.TYPE_RESOLVED;
Expand Down
4 changes: 2 additions & 2 deletions ...c/main/java/org/elasticsearch/xpack/sql/expression/predicate/operator/arithmetic/Sub.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@

import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.expression.predicate.operator.arithmetic.BinaryArithmeticProcessor.BinaryArithmeticOperation;
import org.elasticsearch.xpack.sql.tree.Source;
import org.elasticsearch.xpack.sql.tree.NodeInfo;
import org.elasticsearch.xpack.sql.tree.Source;
import org.elasticsearch.xpack.sql.type.DataTypes;

import static org.elasticsearch.common.logging.LoggerMessageFormat.format;
Expand Down Expand Up @@ -36,7 +36,7 @@ protected Sub replaceChildren(Expression newLeft, Expression newRight) {
protected TypeResolution resolveWithIntervals() {
if (right().dataType().isDateBased() && DataTypes.isInterval(left().dataType())) {
return new TypeResolution(format(null, "Cannot subtract a {}[{}] from an interval[{}]; do you mean the reverse?",
right().dataType().esType, right().source().text(), left().source().text()));
right().dataType().typeName, right().source().text(), left().source().text()));
}
return TypeResolution.TYPE_RESOLVED;
}
Expand Down
2 changes: 1 addition & 1 deletion ...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/ShowColumns.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ private void fillInRows(Map<String, EsField> mapping, String prefix, List<List<?
String name = e.getKey();
if (dt != null) {
// show only fields that exist in ES
rows.add(asList(prefix != null ? prefix + "." + name : name, dt.sqlName(), dt.esType));
rows.add(asList(prefix != null ? prefix + "." + name : name, dt.sqlName(), dt.typeName));
if (field.getProperties().isEmpty() == false) {
String newPrefix = prefix != null ? prefix + "." + name : name;
fillInRows(field.getProperties(), newPrefix, rows);
Expand Down
5 changes: 2 additions & 3 deletions ...in/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumns.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.sql.DatabaseMetaData;
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -143,7 +142,7 @@ static void fillInRows(String clusterName, String indexName, Map<String, EsField
indexName,
name,
odbcCompatible(type.sqlType.getVendorTypeNumber(), isOdbcClient),
type.esType.toUpperCase(Locale.ROOT),
type.toString(),
type.displaySize,
// TODO: is the buffer_length correct?
type.size,
Expand Down Expand Up @@ -208,4 +207,4 @@ public boolean equals(Object obj) {
&& Objects.equals(pattern, other.pattern)
&& Objects.equals(columnPattern, other.columnPattern);
}
}
}
7 changes: 3 additions & 4 deletions ...ugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,14 @@
import org.elasticsearch.xpack.sql.session.Rows;
import org.elasticsearch.xpack.sql.session.SchemaRowSet;
import org.elasticsearch.xpack.sql.session.SqlSession;
import org.elasticsearch.xpack.sql.tree.Source;
import org.elasticsearch.xpack.sql.tree.NodeInfo;
import org.elasticsearch.xpack.sql.tree.Source;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.type.DataTypes;

import java.sql.DatabaseMetaData;
import java.util.Comparator;
import java.util.List;
import java.util.Locale;
import java.util.stream.Stream;

import static java.util.Arrays.asList;
Expand Down Expand Up @@ -81,7 +80,7 @@ public final void execute(SqlSession session, ActionListener<SchemaRowSet> liste
List<List<?>> rows = values
// sort by SQL int type (that's what the JDBC/ODBC specs want) followed by name
.sorted(Comparator.comparing((DataType t) -> t.sqlType.getVendorTypeNumber()).thenComparing(DataType::sqlName))
.map(t -> asList(t.esType.toUpperCase(Locale.ROOT),
.map(t -> asList(t.toString(),
t.sqlType.getVendorTypeNumber(),
//https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/column-size?view=sql-server-2017
t.defaultPrecision,
Expand Down Expand Up @@ -132,4 +131,4 @@ public boolean equals(Object obj) {

return type.equals(((SysTypes) obj).type);
}
}
}
4 changes: 2 additions & 2 deletions .../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlQueryAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,9 @@ static SqlQueryResponse createResponse(SqlQueryRequest request, SchemaRowSet row
List<ColumnInfo> columns = new ArrayList<>(rowSet.columnCount());
for (Schema.Entry entry : rowSet.schema()) {
if (Mode.isDriver(request.mode())) {
columns.add(new ColumnInfo("", entry.name(), entry.type().esType, entry.type().displaySize));
columns.add(new ColumnInfo("", entry.name(), entry.type().typeName, entry.type().displaySize));
} else {
columns.add(new ColumnInfo("", entry.name(), entry.type().esType));
columns.add(new ColumnInfo("", entry.name(), entry.type().typeName));
}
}
columns = unmodifiableList(columns);
Expand Down
2 changes: 1 addition & 1 deletion ...lugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/TopHitsAggRef.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,6 @@ public DataType fieldDataType() {

@Override
public String toString() {
return ">" + name + "[" + fieldDataType.esType + "]";
return ">" + name + "[" + fieldDataType.typeName + "]";
}
}
59 changes: 34 additions & 25 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,41 +23,41 @@
public enum DataType {

// @formatter:off
// jdbc type, size, defPrecision,dispSize, int, rat, docvals
NULL( JDBCType.NULL, 0, 0, 0, false, false, false),
UNSUPPORTED( JDBCType.OTHER, 0, 0, 0, false, false, false),
BOOLEAN( JDBCType.BOOLEAN, 1, 1, 1, false, false, false),
BYTE( JDBCType.TINYINT, Byte.BYTES, 3, 5, true, false, true),
SHORT( JDBCType.SMALLINT, Short.BYTES, 5, 6, true, false, true),
INTEGER( JDBCType.INTEGER, Integer.BYTES, 10, 11, true, false, true),
LONG( JDBCType.BIGINT, Long.BYTES, 19, 20, true, false, true),
// esType jdbc type, size, defPrecision,dispSize, int, rat, docvals
NULL( "null", JDBCType.NULL, 0, 0, 0, false, false, false),
UNSUPPORTED( JDBCType.OTHER, 0, 0, 0, false, false, false),
BOOLEAN( "boolean", JDBCType.BOOLEAN, 1, 1, 1, false, false, false),
BYTE( "byte", JDBCType.TINYINT, Byte.BYTES, 3, 5, true, false, true),
SHORT( "short", JDBCType.SMALLINT, Short.BYTES, 5, 6, true, false, true),
INTEGER( "integer", JDBCType.INTEGER, Integer.BYTES, 10, 11, true, false, true),
LONG( "long", JDBCType.BIGINT, Long.BYTES, 19, 20, true, false, true),
// 53 bits defaultPrecision ~ 15(15.95) decimal digits (53log10(2)),
DOUBLE( JDBCType.DOUBLE, Double.BYTES, 15, 25, false, true, true),
DOUBLE( "double", JDBCType.DOUBLE, Double.BYTES, 15, 25, false, true, true),
// 24 bits defaultPrecision - 24*log10(2) =~ 7 (7.22)
FLOAT( JDBCType.REAL, Float.BYTES, 7, 15, false, true, true),
HALF_FLOAT( JDBCType.FLOAT, Double.BYTES, 16, 25, false, true, true),
FLOAT( "float", JDBCType.REAL, Float.BYTES, 7, 15, false, true, true),
HALF_FLOAT( "half_float", JDBCType.FLOAT, Double.BYTES, 16, 25, false, true, true),
// precision is based on long
SCALED_FLOAT( JDBCType.FLOAT, Double.BYTES, 19, 25, false, true, true),
KEYWORD( JDBCType.VARCHAR, Integer.MAX_VALUE, 256, 0, false, false, true),
TEXT( JDBCType.VARCHAR, Integer.MAX_VALUE, Integer.MAX_VALUE, 0, false, false, false),
OBJECT( JDBCType.STRUCT, -1, 0, 0, false, false, false),
NESTED( JDBCType.STRUCT, -1, 0, 0, false, false, false),
BINARY( JDBCType.VARBINARY, -1, Integer.MAX_VALUE, 0, false, false, false),
DATE( JDBCType.DATE, Long.BYTES, 10, 10, false, false, true),
SCALED_FLOAT( "scaled_float", JDBCType.FLOAT, Double.BYTES, 19, 25, false, true, true),
KEYWORD( "keyword", JDBCType.VARCHAR, Integer.MAX_VALUE, 256, 0, false, false, true),
TEXT( "text", JDBCType.VARCHAR, Integer.MAX_VALUE, Integer.MAX_VALUE, 0, false, false, false),
OBJECT( "object", JDBCType.STRUCT, -1, 0, 0, false, false, false),
NESTED( "nested", JDBCType.STRUCT, -1, 0, 0, false, false, false),
BINARY( "binary", JDBCType.VARBINARY, -1, Integer.MAX_VALUE, 0, false, false, false),
DATE( JDBCType.DATE, Long.BYTES, 10, 10, false, false, true),
// since ODBC and JDBC interpret precision for Date as display size
// the precision is 23 (number of chars in ISO8601 with millis) + Z (the UTC timezone)
// see https://github.com/elastic/elasticsearch/issues/30386#issuecomment-386807288
DATETIME( JDBCType.TIMESTAMP, Long.BYTES, 24, 24, false, false, true),
DATETIME( "date", JDBCType.TIMESTAMP, Long.BYTES, 24, 24, false, false, true),
//
// specialized types
//
// IP can be v4 or v6. The latter has 2^128 addresses or 340,282,366,920,938,463,463,374,607,431,768,211,456
// aka 39 chars
IP( JDBCType.VARCHAR, 39, 39, 0, false, false, true),
IP( "ip", JDBCType.VARCHAR, 39, 39, 0, false, false, true),
//
// INTERVALS
// the list is long as there are a lot of variations and that's what clients (ODBC) expect
// jdbc type, size, prec,disp, int, rat, docvals
// esType:null jdbc type, size, prec,disp, int, rat, docvals
INTERVAL_YEAR( ExtTypes.INTERVAL_YEAR, Integer.BYTES, 7, 7, false, false, false),
INTERVAL_MONTH( ExtTypes.INTERVAL_MONTH, Integer.BYTES, 7, 7, false, false, false),
INTERVAL_DAY( ExtTypes.INTERVAL_DAY, Long.BYTES, 23, 23, false, false, false),
Expand Down Expand Up @@ -126,7 +126,12 @@ public enum DataType {


/**
* Elasticsearch type name
* Type's name used for error messages and column info for the clients
*/
public final String typeName;

/**
* Elasticsearch data type that it maps to
*/
public final String esType;

Expand Down Expand Up @@ -176,7 +181,13 @@ public enum DataType {

DataType(SQLType sqlType, int size, int defaultPrecision, int displaySize, boolean isInteger,
boolean isRational, boolean defaultDocValues) {
this.esType = name().toLowerCase(Locale.ROOT);
this(null, sqlType, size, defaultPrecision, displaySize, isInteger, isRational, defaultDocValues);
}

DataType(String esType, SQLType sqlType, int size, int defaultPrecision, int displaySize, boolean isInteger,
boolean isRational, boolean defaultDocValues) {
this.typeName = name().toLowerCase(Locale.ROOT);
this.esType = esType;
this.sqlType = sqlType;
this.size = size;
this.defaultPrecision = defaultPrecision;
Expand Down Expand Up @@ -228,8 +239,6 @@ public static DataType fromOdbcType(String odbcType) {

/**
* Creates returns DataType enum corresponding to the specified es type
* <p>
* For any dataType DataType.fromTypeName(dataType.esType) == dataType
*/
public static DataType fromTypeName(String esType) {
String uppercase = esType.toUpperCase(Locale.ROOT);
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/Schema.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,9 @@ public String toString() {
}
sb.append(names.get(i));
sb.append(":");
sb.append(types.get(i).esType);
sb.append(types.get(i).typeName);
}
sb.append("]");
return sb.toString();
}
}
}
4 changes: 2 additions & 2 deletions ...ugin/sql/src/test/java/org/elasticsearch/xpack/sql/analysis/index/IndexResolverTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public static Map<String, Map<String, FieldCapabilities>> fromMappings(EsIndex..
if (entry.getValue().size() > 1) {
for (EsIndex index : indices) {
EsField field = index.mapping().get(fieldName);
UpdateableFieldCapabilities fieldCaps = (UpdateableFieldCapabilities) caps.get(field.getDataType().esType);
UpdateableFieldCapabilities fieldCaps = (UpdateableFieldCapabilities) caps.get(field.getDataType().typeName);
fieldCaps.indices.add(index.name());
}
//TODO: what about nonAgg/SearchIndices?
Expand All @@ -171,7 +171,7 @@ private static void addFieldCaps(String parent, EsField field, String indexName,
map = new HashMap<>();
merged.put(fieldName, map);
}
FieldCapabilities caps = map.computeIfAbsent(field.getDataType().esType,
FieldCapabilities caps = map.computeIfAbsent(field.getDataType().typeName,
esType -> new UpdateableFieldCapabilities(fieldName, esType,
isSearchable(field.getDataType()),
isAggregatable(field.getDataType())));
Expand Down
16 changes: 8 additions & 8 deletions x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/ParameterTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class ParameterTests extends ESTestCase {
public void testSingleParameter() {
Expression expression = new SqlParser().createExpression("a = \n?",
Collections.singletonList(
new SqlTypedParamValue(DataType.KEYWORD.esType, "foo")
new SqlTypedParamValue(DataType.KEYWORD.typeName, "foo")
));
logger.info(expression);
assertThat(expression, instanceOf(Equals.class));
Expand All @@ -42,10 +42,10 @@ public void testSingleParameter() {

public void testMultipleParameters() {
Expression expression = new SqlParser().createExpression("(? + ? * ?) - ?", Arrays.asList(
new SqlTypedParamValue(DataType.LONG.esType, 1L),
new SqlTypedParamValue(DataType.LONG.esType, 2L),
new SqlTypedParamValue(DataType.LONG.esType, 3L),
new SqlTypedParamValue(DataType.LONG.esType, 4L)
new SqlTypedParamValue(DataType.LONG.typeName, 1L),
new SqlTypedParamValue(DataType.LONG.typeName, 2L),
new SqlTypedParamValue(DataType.LONG.typeName, 3L),
new SqlTypedParamValue(DataType.LONG.typeName, 4L)
));
assertThat(expression, instanceOf(Sub.class));
Sub sub = (Sub) expression;
Expand All @@ -62,9 +62,9 @@ public void testMultipleParameters() {
public void testNotEnoughParameters() {
ParsingException ex = expectThrows(ParsingException.class,
() -> new SqlParser().createExpression("(? + ? * ?) - ?", Arrays.asList(
new SqlTypedParamValue(DataType.LONG.esType, 1L),
new SqlTypedParamValue(DataType.LONG.esType, 2L),
new SqlTypedParamValue(DataType.LONG.esType, 3L)
new SqlTypedParamValue(DataType.LONG.typeName, 1L),
new SqlTypedParamValue(DataType.LONG.typeName, 2L),
new SqlTypedParamValue(DataType.LONG.typeName, 3L)
)));
assertThat(ex.getMessage(), containsString("Not enough actual parameters"));
}
Expand Down
Loading

0 comments on commit 2c30501

Please sign in to comment.