From 2594c1fb381b396aa13a556109412c4b3a07ca70 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Fri, 11 May 2018 10:17:01 +0300 Subject: [PATCH] SQL: Improve correctness of SYS COLUMNS & TYPES (#30418) Tweak the return data, in particular with regards for ODBC columns to better align with the spec Fix order for SYS TYPES and TABLES according to the JDBC/ODBC spec Fix #30386 Fix #30521 --- .../xpack/sql/type/DataType.java | 11 +-- .../plan/logical/command/sys/SysColumns.java | 12 ++-- .../logical/command/sys/SysTableTypes.java | 3 + .../plan/logical/command/sys/SysTypes.java | 19 ++--- .../xpack/sql/type/DataTypes.java | 69 ++++++++++++++++++- .../xpack/sql/type/DateEsField.java | 7 -- .../logical/command/sys/SysColumnsTests.java | 11 +++ .../logical/command/sys/SysParserTests.java | 6 +- .../command/sys/SysTableTypesTests.java | 4 +- .../xpack/sql/type/DataTypesTests.java | 58 ++++++++++++++++ .../xpack/sql/type/TypesTests.java | 2 +- .../setup_mock_metadata_get_columns.sql | 12 ++-- 12 files changed, 175 insertions(+), 39 deletions(-) create mode 100644 x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java index 95c9ade5e295d..c024af48187d3 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java @@ -25,8 +25,8 @@ public enum DataType { SHORT( JDBCType.SMALLINT, Short.class, Short.BYTES, 5, 6, true, false, true), INTEGER( JDBCType.INTEGER, Integer.class, Integer.BYTES, 10, 11, true, false, true), LONG( JDBCType.BIGINT, Long.class, Long.BYTES, 19, 20, true, false, true), - // 53 bits defaultPrecision ~ 16(15.95) decimal digits (53log10(2)), - DOUBLE( JDBCType.DOUBLE, Double.class, Double.BYTES, 16, 25, false, true, true), + // 53 bits defaultPrecision ~ 15(15.95) decimal digits (53log10(2)), + DOUBLE( JDBCType.DOUBLE, Double.class, Double.BYTES, 15, 25, false, true, true), // 24 bits defaultPrecision - 24*log10(2) =~ 7 (7.22) FLOAT( JDBCType.REAL, Float.class, Float.BYTES, 7, 15, false, true, true), HALF_FLOAT( JDBCType.FLOAT, Double.class, Double.BYTES, 16, 25, false, true, true), @@ -37,7 +37,10 @@ public enum DataType { OBJECT( JDBCType.STRUCT, null, -1, 0, 0), NESTED( JDBCType.STRUCT, null, -1, 0, 0), BINARY( JDBCType.VARBINARY, byte[].class, -1, Integer.MAX_VALUE, 0), - DATE( JDBCType.TIMESTAMP, Timestamp.class, Long.BYTES, 19, 20); + // 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 + DATE( JDBCType.TIMESTAMP, Timestamp.class, Long.BYTES, 24, 24); // @formatter:on private static final Map jdbcToEs; @@ -75,7 +78,7 @@ public enum DataType { *

* Specified column size. For numeric data, this is the maximum precision. For character * data, this is the length in characters. For datetime datatypes, this is the length in characters of the - * String representation (assuming the maximum allowed defaultPrecision of the fractional seconds component). + * String representation (assuming the maximum allowed defaultPrecision of the fractional milliseconds component). */ public final int defaultPrecision; diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumns.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumns.java index 3c01736cebe89..8005ce0758981 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumns.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumns.java @@ -17,6 +17,7 @@ import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; import org.elasticsearch.xpack.sql.type.DataType; +import org.elasticsearch.xpack.sql.type.DataTypes; import org.elasticsearch.xpack.sql.type.EsField; import java.sql.DatabaseMetaData; @@ -29,7 +30,6 @@ import static java.util.Arrays.asList; import static org.elasticsearch.xpack.sql.type.DataType.INTEGER; -import static org.elasticsearch.xpack.sql.type.DataType.NULL; import static org.elasticsearch.xpack.sql.type.DataType.SHORT; /** @@ -133,11 +133,7 @@ static void fillInRows(String clusterName, String indexName, Map output() { @Override public final void execute(SqlSession session, ActionListener listener) { listener.onResponse(Rows.of(output(), IndexType.VALID.stream() + // *DBC requires ascending order + .sorted(Comparator.comparing(t -> t.toSql())) .map(t -> singletonList(t.toSql())) .collect(toList()))); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypes.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypes.java index 508ffef530573..ab40b076fac85 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypes.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypes.java @@ -14,6 +14,7 @@ import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; import org.elasticsearch.xpack.sql.type.DataType; +import org.elasticsearch.xpack.sql.type.DataTypes; import java.sql.DatabaseMetaData; import java.util.Comparator; @@ -67,9 +68,10 @@ public List output() { public final void execute(SqlSession session, ActionListener listener) { List> rows = Stream.of(DataType.values()) // sort by SQL int type (that's what the JDBC/ODBC specs want) - .sorted(Comparator.comparing(t -> t.jdbcType)) + .sorted(Comparator.comparing(t -> t.jdbcType.getVendorTypeNumber())) .map(t -> asList(t.esType.toUpperCase(Locale.ROOT), t.jdbcType.getVendorTypeNumber(), + //https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/column-size?view=sql-server-2017 t.defaultPrecision, "'", "'", @@ -83,16 +85,17 @@ public final void execute(SqlSession session, ActionListener liste // only numerics are signed !t.isSigned(), //no fixed precision scale SQL_FALSE - false, - null, - null, - null, + Boolean.FALSE, + // not auto-incremented + Boolean.FALSE, null, + DataTypes.metaSqlMinimumScale(t), + DataTypes.metaSqlMaximumScale(t), // SQL_DATA_TYPE - ODBC wants this to be not null - 0, - null, + DataTypes.metaSqlDataType(t), + DataTypes.metaSqlDateTimeSub(t), // Radix - t.isInteger ? Integer.valueOf(10) : (t.isRational ? Integer.valueOf(2) : null), + DataTypes.metaSqlRadix(t), null )) .collect(toList()); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java index c2b40656ba294..6fc7f95bef71e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java @@ -51,4 +51,71 @@ public static DataType fromJava(Object value) { } throw new SqlIllegalArgumentException("No idea what's the DataType for {}", value.getClass()); } -} + + // + // Metadata methods, mainly for ODBC. + // As these are fairly obscure and limited in use, there is no point to promote them as a full type methods + // hence why they appear here as utility methods. + // + + // https://docs.microsoft.com/en-us/sql/relational-databases/native-client-odbc-date-time/metadata-catalog + // https://github.com/elastic/elasticsearch/issues/30386 + public static Integer metaSqlDataType(DataType t) { + if (t == DataType.DATE) { + // ODBC SQL_DATETME + return Integer.valueOf(9); + } + // this is safe since the vendor SQL types are short despite the return value + return t.jdbcType.getVendorTypeNumber(); + } + + // https://github.com/elastic/elasticsearch/issues/30386 + // https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgettypeinfo-function?view=sql-server-2017 + public static Integer metaSqlDateTimeSub(DataType t) { + if (t == DataType.DATE) { + // ODBC SQL_CODE_TIMESTAMP + return Integer.valueOf(3); + } + // ODBC null + return 0; + } + + // https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/decimal-digits?view=sql-server-2017 + public static Short metaSqlMinimumScale(DataType t) { + // TODO: return info for HALF/SCALED_FLOATS (should be based on field not type) + if (t == DataType.DATE) { + return Short.valueOf((short) 3); + } + if (t.isInteger) { + return Short.valueOf((short) 0); + } + // minimum scale? + if (t.isRational) { + return Short.valueOf((short) 0); + } + return null; + } + + public static Short metaSqlMaximumScale(DataType t) { + // TODO: return info for HALF/SCALED_FLOATS (should be based on field not type) + if (t == DataType.DATE) { + return Short.valueOf((short) 3); + } + if (t.isInteger) { + return Short.valueOf((short) 0); + } + if (t.isRational) { + return Short.valueOf((short) t.defaultPrecision); + } + return null; + } + + // https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgettypeinfo-function?view=sql-server-2017 + public static Integer metaSqlRadix(DataType t) { + // RADIX - Determines how numbers returned by COLUMN_SIZE and DECIMAL_DIGITS should be interpreted. + // 10 means they represent the number of decimal digits allowed for the column. + // 2 means they represent the number of bits allowed for the column. + // null means radix is not applicable for the given type. + return t.isInteger ? Integer.valueOf(10) : (t.isRational ? Integer.valueOf(2) : null); + } +} \ No newline at end of file diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DateEsField.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DateEsField.java index b9737fbba608f..04926db5407f5 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DateEsField.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DateEsField.java @@ -25,13 +25,6 @@ public DateEsField(String name, Map properties, boolean hasDocV this.formats = CollectionUtils.isEmpty(formats) ? DEFAULT_FORMAT : Arrays.asList(formats); } - @Override - public int getPrecision() { - // same as Long - // TODO: based this on format string - return 19; - } - public List getFormats() { return formats; } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java index bddddc6941cbb..0b82530022386 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java @@ -38,6 +38,13 @@ public void testSysColumns() { assertEquals(null, radix(row)); assertEquals(Integer.MAX_VALUE, bufferLength(row)); + row = rows.get(4); + assertEquals("date", name(row)); + assertEquals(Types.TIMESTAMP, sqlType(row)); + assertEquals(null, radix(row)); + assertEquals(24, precision(row)); + assertEquals(8, bufferLength(row)); + row = rows.get(7); assertEquals("some.dotted", name(row)); assertEquals(Types.STRUCT, sqlType(row)); @@ -59,6 +66,10 @@ private static Object sqlType(List list) { return list.get(4); } + private static Object precision(List list) { + return list.get(6); + } + private static Object bufferLength(List list) { return list.get(7); } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysParserTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysParserTests.java index ac72bcba4d647..27ed27413110f 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysParserTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysParserTests.java @@ -57,8 +57,8 @@ private Tuple sql(String sql) { public void testSysTypes() throws Exception { Command cmd = sql("SYS TYPES").v1(); - List names = asList("BYTE", "SHORT", "INTEGER", "LONG", "HALF_FLOAT", "SCALED_FLOAT", "FLOAT", "DOUBLE", "KEYWORD", "TEXT", - "DATE", "BINARY", "NULL", "UNSUPPORTED", "OBJECT", "NESTED", "BOOLEAN"); + List names = asList("BYTE", "LONG", "BINARY", "NULL", "INTEGER", "SHORT", "HALF_FLOAT", "SCALED_FLOAT", "FLOAT", "DOUBLE", + "KEYWORD", "TEXT", "BOOLEAN", "DATE", "UNSUPPORTED", "OBJECT", "NESTED"); cmd.execute(null, ActionListener.wrap(r -> { assertEquals(19, r.columnCount()); @@ -68,6 +68,8 @@ public void testSysTypes() throws Exception { assertFalse(r.column(9, Boolean.class)); // make sure precision is returned as boolean (not int) assertFalse(r.column(10, Boolean.class)); + // no auto-increment + assertFalse(r.column(11, Boolean.class)); for (int i = 0; i < r.size(); i++) { assertEquals(names.get(i), r.column(0)); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypesTests.java index 956273b9aae2d..291f9ee244e5f 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypesTests.java @@ -41,9 +41,9 @@ public void testSysCatalogs() throws Exception { sql.v1().execute(sql.v2(), ActionListener.wrap(r -> { assertEquals(2, r.size()); - assertEquals("BASE TABLE", r.column(0)); - r.advanceRow(); assertEquals("ALIAS", r.column(0)); + r.advanceRow(); + assertEquals("BASE TABLE", r.column(0)); }, ex -> fail(ex.getMessage()))); } } \ No newline at end of file diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java new file mode 100644 index 0000000000000..0a34c697bdf64 --- /dev/null +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java @@ -0,0 +1,58 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.sql.type; + +import org.elasticsearch.test.ESTestCase; + +import static org.elasticsearch.xpack.sql.type.DataType.DATE; +import static org.elasticsearch.xpack.sql.type.DataType.FLOAT; +import static org.elasticsearch.xpack.sql.type.DataType.KEYWORD; +import static org.elasticsearch.xpack.sql.type.DataType.LONG; +import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlDataType; +import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlDateTimeSub; +import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlMaximumScale; +import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlMinimumScale; +import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlRadix; + +public class DataTypesTests extends ESTestCase { + + public void testMetaDataType() { + assertEquals(Integer.valueOf(9), metaSqlDataType(DATE)); + DataType t = randomDataTypeNoDate(); + assertEquals(t.jdbcType.getVendorTypeNumber(), metaSqlDataType(t)); + } + + public void testMetaDateTypeSub() { + assertEquals(Integer.valueOf(3), metaSqlDateTimeSub(DATE)); + assertEquals(Integer.valueOf(0), metaSqlDateTimeSub(randomDataTypeNoDate())); + } + + public void testMetaMinimumScale() { + assertEquals(Short.valueOf((short) 3), metaSqlMinimumScale(DATE)); + assertEquals(Short.valueOf((short) 0), metaSqlMinimumScale(LONG)); + assertEquals(Short.valueOf((short) 0), metaSqlMinimumScale(FLOAT)); + assertNull(metaSqlMinimumScale(KEYWORD)); + } + + public void testMetaMaximumScale() { + assertEquals(Short.valueOf((short) 3), metaSqlMaximumScale(DATE)); + assertEquals(Short.valueOf((short) 0), metaSqlMaximumScale(LONG)); + assertEquals(Short.valueOf((short) FLOAT.defaultPrecision), metaSqlMaximumScale(FLOAT)); + assertNull(metaSqlMaximumScale(KEYWORD)); + } + + public void testMetaRadix() { + assertNull(metaSqlRadix(DATE)); + assertNull(metaSqlRadix(KEYWORD)); + assertEquals(Integer.valueOf(10), metaSqlRadix(LONG)); + assertEquals(Integer.valueOf(2), metaSqlRadix(FLOAT)); + } + + private DataType randomDataTypeNoDate() { + return randomValueOtherThan(DataType.DATE, () -> randomFrom(DataType.values())); + } +} + diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/TypesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/TypesTests.java index c5e82123d7b8b..891b11ba70bb0 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/TypesTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/TypesTests.java @@ -82,7 +82,7 @@ public void testDateField() { EsField field = mapping.get("date"); assertThat(field.getDataType(), is(DATE)); assertThat(field.hasDocValues(), is(true)); - assertThat(field.getPrecision(), is(19)); + assertThat(field.getPrecision(), is(24)); DateEsField dfield = (DateEsField) field; List formats = dfield.getFormats(); diff --git a/x-pack/qa/sql/src/main/resources/setup_mock_metadata_get_columns.sql b/x-pack/qa/sql/src/main/resources/setup_mock_metadata_get_columns.sql index 3d8cf4708945e..69c572f4ddd4e 100644 --- a/x-pack/qa/sql/src/main/resources/setup_mock_metadata_get_columns.sql +++ b/x-pack/qa/sql/src/main/resources/setup_mock_metadata_get_columns.sql @@ -25,26 +25,26 @@ CREATE TABLE mock ( ) AS SELECT null, 'test1', 'name', 12, 'TEXT', 0, 2147483647, null, null, 1, -- columnNullable - null, null, 12, null, 2147483647, 1, 'YES', null, null, null, null, 'NO', 'NO' + null, null, 12, 0, 2147483647, 1, 'YES', null, null, null, null, 'NO', 'NO' FROM DUAL UNION ALL SELECT null, 'test1', 'name.keyword', 12, 'KEYWORD', 0, 2147483647, null, null, 1, -- columnNullable - null, null, 12, null, 2147483647, 1, 'YES', null, null, null, null, 'NO', 'NO' + null, null, 12, 0, 2147483647, 1, 'YES', null, null, null, null, 'NO', 'NO' FROM DUAL UNION ALL -SELECT null, 'test2', 'date', 93, 'DATE', 20, 8, null, null, +SELECT null, 'test2', 'date', 93, 'DATE', 24, 8, null, null, 1, -- columnNullable - null, null, 93, null, null, 1, 'YES', null, null, null, null, 'NO', 'NO' + null, null, 9, 3, null, 1, 'YES', null, null, null, null, 'NO', 'NO' FROM DUAL UNION ALL SELECT null, 'test2', 'float', 7, 'FLOAT', 15, 4, null, 2, 1, -- columnNullable - null, null, 7, null, null, 2, 'YES', null, null, null, null, 'NO', 'NO' + null, null, 7, 0, null, 2, 'YES', null, null, null, null, 'NO', 'NO' FROM DUAL UNION ALL SELECT null, 'test2', 'number', -5, 'LONG', 20, 8, null, 10, 1, -- columnNullable - null, null, -5, null, null, 3, 'YES', null, null, null, null, 'NO', 'NO' + null, null, -5, 0, null, 3, 'YES', null, null, null, null, 'NO', 'NO' FROM DUAL ;