Skip to content

Commit 2594c1f

Browse files
costincostin
authored
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
1 parent 73b08d9 commit 2594c1f

File tree

12 files changed

+175
-39
lines changed

12 files changed

+175
-39
lines changed

‎x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/type/DataType.java

+7-4
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ public enum DataType {
2525
SHORT( JDBCType.SMALLINT, Short.class, Short.BYTES, 5, 6, true, false, true),
2626
INTEGER( JDBCType.INTEGER, Integer.class, Integer.BYTES, 10, 11, true, false, true),
2727
LONG( JDBCType.BIGINT, Long.class, Long.BYTES, 19, 20, true, false, true),
28-
// 53 bits defaultPrecision ~ 16(15.95) decimal digits (53log10(2)),
29-
DOUBLE( JDBCType.DOUBLE, Double.class, Double.BYTES, 16, 25, false, true, true),
28+
// 53 bits defaultPrecision ~ 15(15.95) decimal digits (53log10(2)),
29+
DOUBLE( JDBCType.DOUBLE, Double.class, Double.BYTES, 15, 25, false, true, true),
3030
// 24 bits defaultPrecision - 24*log10(2) =~ 7 (7.22)
3131
FLOAT( JDBCType.REAL, Float.class, Float.BYTES, 7, 15, false, true, true),
3232
HALF_FLOAT( JDBCType.FLOAT, Double.class, Double.BYTES, 16, 25, false, true, true),
@@ -37,7 +37,10 @@ public enum DataType {
3737
OBJECT( JDBCType.STRUCT, null, -1, 0, 0),
3838
NESTED( JDBCType.STRUCT, null, -1, 0, 0),
3939
BINARY( JDBCType.VARBINARY, byte[].class, -1, Integer.MAX_VALUE, 0),
40-
DATE( JDBCType.TIMESTAMP, Timestamp.class, Long.BYTES, 19, 20);
40+
// since ODBC and JDBC interpret precision for Date as display size,
41+
// the precision is 23 (number of chars in ISO8601 with millis) + Z (the UTC timezone)
42+
// see https://github.com/elastic/elasticsearch/issues/30386#issuecomment-386807288
43+
DATE( JDBCType.TIMESTAMP, Timestamp.class, Long.BYTES, 24, 24);
4144
// @formatter:on
4245

4346
private static final Map<JDBCType, DataType> jdbcToEs;
@@ -75,7 +78,7 @@ public enum DataType {
7578
* <p>
7679
* Specified column size. For numeric data, this is the maximum precision. For character
7780
* data, this is the length in characters. For datetime datatypes, this is the length in characters of the
78-
* String representation (assuming the maximum allowed defaultPrecision of the fractional seconds component).
81+
* String representation (assuming the maximum allowed defaultPrecision of the fractional milliseconds component).
7982
*/
8083
public final int defaultPrecision;
8184

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumns.java

+4-8
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import org.elasticsearch.xpack.sql.tree.Location;
1818
import org.elasticsearch.xpack.sql.tree.NodeInfo;
1919
import org.elasticsearch.xpack.sql.type.DataType;
20+
import org.elasticsearch.xpack.sql.type.DataTypes;
2021
import org.elasticsearch.xpack.sql.type.EsField;
2122

2223
import java.sql.DatabaseMetaData;
@@ -29,7 +30,6 @@
2930

3031
import static java.util.Arrays.asList;
3132
import static org.elasticsearch.xpack.sql.type.DataType.INTEGER;
32-
import static org.elasticsearch.xpack.sql.type.DataType.NULL;
3333
import static org.elasticsearch.xpack.sql.type.DataType.SHORT;
3434

3535
/**
@@ -133,21 +133,17 @@ static void fillInRows(String clusterName, String indexName, Map<String, EsField
133133
type.size,
134134
// no DECIMAL support
135135
null,
136-
// RADIX - Determines how numbers returned by COLUMN_SIZE and DECIMAL_DIGITS should be interpreted.
137-
// 10 means they represent the number of decimal digits allowed for the column.
138-
// 2 means they represent the number of bits allowed for the column.
139-
// null means radix is not applicable for the given type.
140-
type.isInteger ? Integer.valueOf(10) : type.isRational ? Integer.valueOf(2) : null,
136+
DataTypes.metaSqlRadix(type),
141137
// everything is nullable
142138
DatabaseMetaData.columnNullable,
143139
// no remarks
144140
null,
145141
// no column def
146142
null,
147143
// SQL_DATA_TYPE apparently needs to be same as DATA_TYPE except for datetime and interval data types
148-
type.jdbcType.getVendorTypeNumber(),
144+
DataTypes.metaSqlDataType(type),
149145
// SQL_DATETIME_SUB ?
150-
null,
146+
DataTypes.metaSqlDateTimeSub(type),
151147
// char octet length
152148
type.isString() || type == DataType.BINARY ? type.size : null,
153149
// position

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypes.java

+3
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.elasticsearch.xpack.sql.tree.Location;
1616
import org.elasticsearch.xpack.sql.tree.NodeInfo;
1717

18+
import java.util.Comparator;
1819
import java.util.List;
1920

2021
import static java.util.Collections.singletonList;
@@ -43,6 +44,8 @@ public List<Attribute> output() {
4344
@Override
4445
public final void execute(SqlSession session, ActionListener<SchemaRowSet> listener) {
4546
listener.onResponse(Rows.of(output(), IndexType.VALID.stream()
47+
// *DBC requires ascending order
48+
.sorted(Comparator.comparing(t -> t.toSql()))
4649
.map(t -> singletonList(t.toSql()))
4750
.collect(toList())));
4851
}

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTypes.java

+11-8
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.elasticsearch.xpack.sql.tree.Location;
1515
import org.elasticsearch.xpack.sql.tree.NodeInfo;
1616
import org.elasticsearch.xpack.sql.type.DataType;
17+
import org.elasticsearch.xpack.sql.type.DataTypes;
1718

1819
import java.sql.DatabaseMetaData;
1920
import java.util.Comparator;
@@ -67,9 +68,10 @@ public List<Attribute> output() {
6768
public final void execute(SqlSession session, ActionListener<SchemaRowSet> listener) {
6869
List<List<?>> rows = Stream.of(DataType.values())
6970
// sort by SQL int type (that's what the JDBC/ODBC specs want)
70-
.sorted(Comparator.comparing(t -> t.jdbcType))
71+
.sorted(Comparator.comparing(t -> t.jdbcType.getVendorTypeNumber()))
7172
.map(t -> asList(t.esType.toUpperCase(Locale.ROOT),
7273
t.jdbcType.getVendorTypeNumber(),
74+
//https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/column-size?view=sql-server-2017
7375
t.defaultPrecision,
7476
"'",
7577
"'",
@@ -83,16 +85,17 @@ public final void execute(SqlSession session, ActionListener<SchemaRowSet> liste
8385
// only numerics are signed
8486
!t.isSigned(),
8587
//no fixed precision scale SQL_FALSE
86-
false,
87-
null,
88-
null,
89-
null,
88+
Boolean.FALSE,
89+
// not auto-incremented
90+
Boolean.FALSE,
9091
null,
92+
DataTypes.metaSqlMinimumScale(t),
93+
DataTypes.metaSqlMaximumScale(t),
9194
// SQL_DATA_TYPE - ODBC wants this to be not null
92-
0,
93-
null,
95+
DataTypes.metaSqlDataType(t),
96+
DataTypes.metaSqlDateTimeSub(t),
9497
// Radix
95-
t.isInteger ? Integer.valueOf(10) : (t.isRational ? Integer.valueOf(2) : null),
98+
DataTypes.metaSqlRadix(t),
9699
null
97100
))
98101
.collect(toList());

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DataTypes.java

+68-1
Original file line numberDiff line numberDiff line change
@@ -51,4 +51,71 @@ public static DataType fromJava(Object value) {
5151
}
5252
throw new SqlIllegalArgumentException("No idea what's the DataType for {}", value.getClass());
5353
}
54-
}
54+
55+
//
56+
// Metadata methods, mainly for ODBC.
57+
// As these are fairly obscure and limited in use, there is no point to promote them as a full type methods
58+
// hence why they appear here as utility methods.
59+
//
60+
61+
// https://docs.microsoft.com/en-us/sql/relational-databases/native-client-odbc-date-time/metadata-catalog
62+
// https://github.com/elastic/elasticsearch/issues/30386
63+
public static Integer metaSqlDataType(DataType t) {
64+
if (t == DataType.DATE) {
65+
// ODBC SQL_DATETME
66+
return Integer.valueOf(9);
67+
}
68+
// this is safe since the vendor SQL types are short despite the return value
69+
return t.jdbcType.getVendorTypeNumber();
70+
}
71+
72+
// https://github.com/elastic/elasticsearch/issues/30386
73+
// https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgettypeinfo-function?view=sql-server-2017
74+
public static Integer metaSqlDateTimeSub(DataType t) {
75+
if (t == DataType.DATE) {
76+
// ODBC SQL_CODE_TIMESTAMP
77+
return Integer.valueOf(3);
78+
}
79+
// ODBC null
80+
return 0;
81+
}
82+
83+
// https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/decimal-digits?view=sql-server-2017
84+
public static Short metaSqlMinimumScale(DataType t) {
85+
// TODO: return info for HALF/SCALED_FLOATS (should be based on field not type)
86+
if (t == DataType.DATE) {
87+
return Short.valueOf((short) 3);
88+
}
89+
if (t.isInteger) {
90+
return Short.valueOf((short) 0);
91+
}
92+
// minimum scale?
93+
if (t.isRational) {
94+
return Short.valueOf((short) 0);
95+
}
96+
return null;
97+
}
98+
99+
public static Short metaSqlMaximumScale(DataType t) {
100+
// TODO: return info for HALF/SCALED_FLOATS (should be based on field not type)
101+
if (t == DataType.DATE) {
102+
return Short.valueOf((short) 3);
103+
}
104+
if (t.isInteger) {
105+
return Short.valueOf((short) 0);
106+
}
107+
if (t.isRational) {
108+
return Short.valueOf((short) t.defaultPrecision);
109+
}
110+
return null;
111+
}
112+
113+
// https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgettypeinfo-function?view=sql-server-2017
114+
public static Integer metaSqlRadix(DataType t) {
115+
// RADIX - Determines how numbers returned by COLUMN_SIZE and DECIMAL_DIGITS should be interpreted.
116+
// 10 means they represent the number of decimal digits allowed for the column.
117+
// 2 means they represent the number of bits allowed for the column.
118+
// null means radix is not applicable for the given type.
119+
return t.isInteger ? Integer.valueOf(10) : (t.isRational ? Integer.valueOf(2) : null);
120+
}
121+
}

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/type/DateEsField.java

-7
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,6 @@ public DateEsField(String name, Map<String, EsField> properties, boolean hasDocV
2525
this.formats = CollectionUtils.isEmpty(formats) ? DEFAULT_FORMAT : Arrays.asList(formats);
2626
}
2727

28-
@Override
29-
public int getPrecision() {
30-
// same as Long
31-
// TODO: based this on format string
32-
return 19;
33-
}
34-
3528
public List<String> getFormats() {
3629
return formats;
3730
}

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysColumnsTests.java

+11
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,13 @@ public void testSysColumns() {
3838
assertEquals(null, radix(row));
3939
assertEquals(Integer.MAX_VALUE, bufferLength(row));
4040

41+
row = rows.get(4);
42+
assertEquals("date", name(row));
43+
assertEquals(Types.TIMESTAMP, sqlType(row));
44+
assertEquals(null, radix(row));
45+
assertEquals(24, precision(row));
46+
assertEquals(8, bufferLength(row));
47+
4148
row = rows.get(7);
4249
assertEquals("some.dotted", name(row));
4350
assertEquals(Types.STRUCT, sqlType(row));
@@ -59,6 +66,10 @@ private static Object sqlType(List<?> list) {
5966
return list.get(4);
6067
}
6168

69+
private static Object precision(List<?> list) {
70+
return list.get(6);
71+
}
72+
6273
private static Object bufferLength(List<?> list) {
6374
return list.get(7);
6475
}

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysParserTests.java

+4-2
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ private Tuple<Command, SqlSession> sql(String sql) {
5757
public void testSysTypes() throws Exception {
5858
Command cmd = sql("SYS TYPES").v1();
5959

60-
List<String> names = asList("BYTE", "SHORT", "INTEGER", "LONG", "HALF_FLOAT", "SCALED_FLOAT", "FLOAT", "DOUBLE", "KEYWORD", "TEXT",
61-
"DATE", "BINARY", "NULL", "UNSUPPORTED", "OBJECT", "NESTED", "BOOLEAN");
60+
List<String> names = asList("BYTE", "LONG", "BINARY", "NULL", "INTEGER", "SHORT", "HALF_FLOAT", "SCALED_FLOAT", "FLOAT", "DOUBLE",
61+
"KEYWORD", "TEXT", "BOOLEAN", "DATE", "UNSUPPORTED", "OBJECT", "NESTED");
6262

6363
cmd.execute(null, ActionListener.wrap(r -> {
6464
assertEquals(19, r.columnCount());
@@ -68,6 +68,8 @@ public void testSysTypes() throws Exception {
6868
assertFalse(r.column(9, Boolean.class));
6969
// make sure precision is returned as boolean (not int)
7070
assertFalse(r.column(10, Boolean.class));
71+
// no auto-increment
72+
assertFalse(r.column(11, Boolean.class));
7173

7274
for (int i = 0; i < r.size(); i++) {
7375
assertEquals(names.get(i), r.column(0));

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/command/sys/SysTableTypesTests.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,9 @@ public void testSysCatalogs() throws Exception {
4141

4242
sql.v1().execute(sql.v2(), ActionListener.wrap(r -> {
4343
assertEquals(2, r.size());
44-
assertEquals("BASE TABLE", r.column(0));
45-
r.advanceRow();
4644
assertEquals("ALIAS", r.column(0));
45+
r.advanceRow();
46+
assertEquals("BASE TABLE", r.column(0));
4747
}, ex -> fail(ex.getMessage())));
4848
}
4949
}

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/DataTypesTests.java

+58
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
package org.elasticsearch.xpack.sql.type;
7+
8+
import org.elasticsearch.test.ESTestCase;
9+
10+
import static org.elasticsearch.xpack.sql.type.DataType.DATE;
11+
import static org.elasticsearch.xpack.sql.type.DataType.FLOAT;
12+
import static org.elasticsearch.xpack.sql.type.DataType.KEYWORD;
13+
import static org.elasticsearch.xpack.sql.type.DataType.LONG;
14+
import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlDataType;
15+
import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlDateTimeSub;
16+
import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlMaximumScale;
17+
import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlMinimumScale;
18+
import static org.elasticsearch.xpack.sql.type.DataTypes.metaSqlRadix;
19+
20+
public class DataTypesTests extends ESTestCase {
21+
22+
public void testMetaDataType() {
23+
assertEquals(Integer.valueOf(9), metaSqlDataType(DATE));
24+
DataType t = randomDataTypeNoDate();
25+
assertEquals(t.jdbcType.getVendorTypeNumber(), metaSqlDataType(t));
26+
}
27+
28+
public void testMetaDateTypeSub() {
29+
assertEquals(Integer.valueOf(3), metaSqlDateTimeSub(DATE));
30+
assertEquals(Integer.valueOf(0), metaSqlDateTimeSub(randomDataTypeNoDate()));
31+
}
32+
33+
public void testMetaMinimumScale() {
34+
assertEquals(Short.valueOf((short) 3), metaSqlMinimumScale(DATE));
35+
assertEquals(Short.valueOf((short) 0), metaSqlMinimumScale(LONG));
36+
assertEquals(Short.valueOf((short) 0), metaSqlMinimumScale(FLOAT));
37+
assertNull(metaSqlMinimumScale(KEYWORD));
38+
}
39+
40+
public void testMetaMaximumScale() {
41+
assertEquals(Short.valueOf((short) 3), metaSqlMaximumScale(DATE));
42+
assertEquals(Short.valueOf((short) 0), metaSqlMaximumScale(LONG));
43+
assertEquals(Short.valueOf((short) FLOAT.defaultPrecision), metaSqlMaximumScale(FLOAT));
44+
assertNull(metaSqlMaximumScale(KEYWORD));
45+
}
46+
47+
public void testMetaRadix() {
48+
assertNull(metaSqlRadix(DATE));
49+
assertNull(metaSqlRadix(KEYWORD));
50+
assertEquals(Integer.valueOf(10), metaSqlRadix(LONG));
51+
assertEquals(Integer.valueOf(2), metaSqlRadix(FLOAT));
52+
}
53+
54+
private DataType randomDataTypeNoDate() {
55+
return randomValueOtherThan(DataType.DATE, () -> randomFrom(DataType.values()));
56+
}
57+
}
58+

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/type/TypesTests.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ public void testDateField() {
8282
EsField field = mapping.get("date");
8383
assertThat(field.getDataType(), is(DATE));
8484
assertThat(field.hasDocValues(), is(true));
85-
assertThat(field.getPrecision(), is(19));
85+
assertThat(field.getPrecision(), is(24));
8686

8787
DateEsField dfield = (DateEsField) field;
8888
List<String> formats = dfield.getFormats();

‎x-pack/qa/sql/src/main/resources/setup_mock_metadata_get_columns.sql

+6-6
Original file line numberDiff line numberDiff line change
@@ -25,26 +25,26 @@ CREATE TABLE mock (
2525
) AS
2626
SELECT null, 'test1', 'name', 12, 'TEXT', 0, 2147483647, null, null,
2727
1, -- columnNullable
28-
null, null, 12, null, 2147483647, 1, 'YES', null, null, null, null, 'NO', 'NO'
28+
null, null, 12, 0, 2147483647, 1, 'YES', null, null, null, null, 'NO', 'NO'
2929
FROM DUAL
3030
UNION ALL
3131
SELECT null, 'test1', 'name.keyword', 12, 'KEYWORD', 0, 2147483647, null, null,
3232
1, -- columnNullable
33-
null, null, 12, null, 2147483647, 1, 'YES', null, null, null, null, 'NO', 'NO'
33+
null, null, 12, 0, 2147483647, 1, 'YES', null, null, null, null, 'NO', 'NO'
3434
FROM DUAL
3535
UNION ALL
36-
SELECT null, 'test2', 'date', 93, 'DATE', 20, 8, null, null,
36+
SELECT null, 'test2', 'date', 93, 'DATE', 24, 8, null, null,
3737
1, -- columnNullable
38-
null, null, 93, null, null, 1, 'YES', null, null, null, null, 'NO', 'NO'
38+
null, null, 9, 3, null, 1, 'YES', null, null, null, null, 'NO', 'NO'
3939
FROM DUAL
4040
UNION ALL
4141
SELECT null, 'test2', 'float', 7, 'FLOAT', 15, 4, null, 2,
4242
1, -- columnNullable
43-
null, null, 7, null, null, 2, 'YES', null, null, null, null, 'NO', 'NO'
43+
null, null, 7, 0, null, 2, 'YES', null, null, null, null, 'NO', 'NO'
4444
FROM DUAL
4545
UNION ALL
4646
SELECT null, 'test2', 'number', -5, 'LONG', 20, 8, null, 10,
4747
1, -- columnNullable
48-
null, null, -5, null, null, 3, 'YES', null, null, null, null, 'NO', 'NO'
48+
null, null, -5, 0, null, 3, 'YES', null, null, null, null, 'NO', 'NO'
4949
FROM DUAL
5050
;

0 commit comments

Comments
 (0)