From 24fc863e0c7823a40a61cb5ab0c7c08b0fb6185a Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Sat, 5 May 2018 23:54:45 +0300 Subject: [PATCH 1/6] SQL: Improve correctness of SYS COLUMNS & TYPES Tweak the return data, in particular with regards for ODBC columns to better align with the spec Fix #30386 --- .../xpack/sql/type/DataType.java | 8 ++- .../plan/logical/command/sys/SysColumns.java | 12 ++-- .../plan/logical/command/sys/SysTypes.java | 12 ++-- .../xpack/sql/type/DataTypes.java | 69 +++++++++++++++++- .../xpack/sql/type/DataTypesTests.java | 70 +++++++++++++++++++ 5 files changed, 154 insertions(+), 17 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..e711bc6845265 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,9 @@ 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 19 (number of digits) + Z (the UTC timezone) + // see https://github.com/elastic/elasticsearch/issues/30386#issuecomment-386807288 + DATE( JDBCType.TIMESTAMP, Timestamp.class, Long.BYTES, 20, 20); // @formatter:on private static final Map jdbcToEs; 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 liste .sorted(Comparator.comparing(t -> t.jdbcType)) .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, "'", "'", @@ -86,13 +88,13 @@ public final void execute(SqlSession session, ActionListener liste false, null, null, - null, - 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/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..dd57200c6bb6e --- /dev/null +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java @@ -0,0 +1,70 @@ +/* + * ELASTICSEARCH CONFIDENTIAL + * __________________ + * + * [2017] Elasticsearch Incorporated. All Rights Reserved. + * + * NOTICE: All information contained herein is, and remains + * the property of Elasticsearch Incorporated and its suppliers, + * if any. The intellectual and technical concepts contained + * herein are proprietary to Elasticsearch Incorporated + * and its suppliers and may be covered by U.S. and Foreign Patents, + * patents in process, and are protected by trade secret or copyright law. + * Dissemination of this information or reproduction of this material + * is strictly forbidden unless prior written permission is obtained + * from Elasticsearch Incorporated. + */ + +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())); + } +} + From 19d4700ec84e021bb89d725bb023af1877724651 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Mon, 7 May 2018 10:26:44 +0300 Subject: [PATCH 2/6] Update changelog --- docs/CHANGELOG.asciidoc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index 03811ae85c76c..4012eed9d049d 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -111,6 +111,9 @@ Rollup:: * Validate timezone in range queries to ensure they match the selected job when searching ({pull}30338[#30338]) +SQL:: +* Improve correctness of SYS COLUMNS & SYS TYPES commands ({pull}30418[#30418]) + [float] === Regressions Fail snapshot operations early when creating or deleting a snapshot on a repository that has been @@ -180,6 +183,9 @@ Rollup:: * Validate timezone in range queries to ensure they match the selected job when searching ({pull}30338[#30338]) +SQL:: +* Improve correctness of SYS COLUMNS & SYS TYPES commands ({pull}30418[#30418]) + //[float] //=== Regressions From 45096c9b82d9eb928a4bd272af95c7e7a52f705e Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Mon, 7 May 2018 12:03:01 +0300 Subject: [PATCH 3/6] Fix license header and update precision for Dates --- .../elasticsearch/xpack/sql/type/DataType.java | 7 ++++--- .../xpack/sql/type/DateEsField.java | 7 ------- .../logical/command/sys/SysColumnsTests.java | 11 +++++++++++ .../xpack/sql/type/DataTypesTests.java | 18 +++--------------- .../xpack/sql/type/TypesTests.java | 2 +- 5 files changed, 19 insertions(+), 26 deletions(-) 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 e711bc6845265..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 @@ -37,9 +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), - // since ODBC and JDBC interpret precision for Date as display size, the precision is 19 (number of digits) + Z (the UTC timezone) + // 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, 20, 20); + DATE( JDBCType.TIMESTAMP, Timestamp.class, Long.BYTES, 24, 24); // @formatter:on private static final Map jdbcToEs; @@ -77,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/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/type/DataTypesTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java index dd57200c6bb6e..0a34c697bdf64 100644 --- 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 @@ -1,20 +1,8 @@ /* - * ELASTICSEARCH CONFIDENTIAL - * __________________ - * - * [2017] Elasticsearch Incorporated. All Rights Reserved. - * - * NOTICE: All information contained herein is, and remains - * the property of Elasticsearch Incorporated and its suppliers, - * if any. The intellectual and technical concepts contained - * herein are proprietary to Elasticsearch Incorporated - * and its suppliers and may be covered by U.S. and Foreign Patents, - * patents in process, and are protected by trade secret or copyright law. - * Dissemination of this information or reproduction of this material - * is strictly forbidden unless prior written permission is obtained - * from Elasticsearch Incorporated. + * 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; 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(); From 8a2077aae52535623f5a7683b950eadd6b9eb75c Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Mon, 7 May 2018 15:47:25 +0300 Subject: [PATCH 4/6] A bit more tweaking of returned values --- .../xpack/sql/plan/logical/command/sys/SysTypes.java | 5 +++-- .../xpack/sql/plan/logical/command/sys/SysParserTests.java | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) 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 137e21b761308..2f6c2181c27ba 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 @@ -85,8 +85,9 @@ public final void execute(SqlSession session, ActionListener liste // only numerics are signed !t.isSigned(), //no fixed precision scale SQL_FALSE - false, - null, + Boolean.FALSE, + // not auto-incremented + Boolean.FALSE, null, DataTypes.metaSqlMinimumScale(t), DataTypes.metaSqlMaximumScale(t), 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..5d694b36965dd 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 @@ -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)); From 74870f382014a9069912d7d63608ba84f9dd206d Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Thu, 10 May 2018 16:28:01 +0300 Subject: [PATCH 5/6] Update metadata test --- .../resources/setup_mock_metadata_get_columns.sql | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 ; From 61f4eb5c214962f2a3b2ddb88fd25722c3127c67 Mon Sep 17 00:00:00 2001 From: Costin Leau Date: Thu, 10 May 2018 20:26:46 +0300 Subject: [PATCH 6/6] Fix order for SYS TYPES and TABLES according to the JDBC/ODBC spec Fix #30521 --- docs/CHANGELOG.asciidoc | 2 -- .../xpack/sql/plan/logical/command/sys/SysTableTypes.java | 3 +++ .../xpack/sql/plan/logical/command/sys/SysTypes.java | 2 +- .../xpack/sql/plan/logical/command/sys/SysParserTests.java | 4 ++-- .../sql/plan/logical/command/sys/SysTableTypesTests.java | 4 ++-- 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index 5b8b32b0617c2..f606ae76aa075 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -116,7 +116,6 @@ Rollup:: searching ({pull}30338[#30338]) SQL:: -* Improve correctness of SYS COLUMNS & SYS TYPES commands ({pull}30418[#30418]) * Fix parsing of Dates containing milliseconds ({pull}30419[#30419]) [float] @@ -206,7 +205,6 @@ Rollup:: searching ({pull}30338[#30338]) SQL:: -* Improve correctness of SYS COLUMNS & SYS TYPES commands ({pull}30418[#30418]) * Fix parsing of Dates containing milliseconds ({pull}30419[#30419]) Allocation:: diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypes.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypes.java index ff6789bc3731d..9d7c73b2b8369 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypes.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypes.java @@ -15,6 +15,7 @@ import org.elasticsearch.xpack.sql.tree.Location; import org.elasticsearch.xpack.sql.tree.NodeInfo; +import java.util.Comparator; import java.util.List; import static java.util.Collections.singletonList; @@ -43,6 +44,8 @@ public List 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 2f6c2181c27ba..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 @@ -68,7 +68,7 @@ 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 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 5d694b36965dd..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()); 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