SQL: Improve correctness of SYS COLUMNS & TYPES

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
 

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<JDBCType, DataType> jdbcToEs;

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,21 +133,17 @@ static void fillInRows(String clusterName, String indexName, Map<String, EsField
                         type.size,
                         // no DECIMAL support
                         null,
-                        // 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.
-                        type.isInteger ? Integer.valueOf(10) : type.isRational ? Integer.valueOf(2) : null,
+                        DataTypes.metaSqlRadix(type),
                         // everything is nullable
                         DatabaseMetaData.columnNullable,
                         // no remarks
                         null,
                         // no column def
                         null,
                         // SQL_DATA_TYPE apparently needs to be same as DATA_TYPE except for datetime and interval data types
-                        type.jdbcType.getVendorTypeNumber(),
+                        DataTypes.metaSqlDataType(type),
                         // SQL_DATETIME_SUB ?
-                        null,
+                        DataTypes.metaSqlDateTimeSub(type),
                         // char octet length
                         type.isString() || type == DataType.BINARY ? type.size : null,
                         // position

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;
@@ -70,6 +71,7 @@ public final void execute(SqlSession session, ActionListener<SchemaRowSet> 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<SchemaRowSet> 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());

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);
+    }
+}

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()));
+    }
+}
+