SQL: Adjust the precision and scale for drivers #40467
Conversation
|
Pinging @elastic/es-search |
|
@bpintea can you please double check the changes? Thanks! |
| 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), | ||
| SCALED_FLOAT( "scaled_float", JDBCType.DOUBLE, Integer.BYTES, 15, 25, false, true, true), | ||
| KEYWORD( "keyword", JDBCType.VARCHAR, Integer.MAX_VALUE, 256, 256,false, false, true), |
There was a problem hiding this comment.
I wonder if 256 is correct here. I think that's the default, but IIRC, this is a setting and thus could have a different (maximum) value. Treating KEYWORD as TEXT in this regard might be potentially safer?
It otherwise LGTM.
There was a problem hiding this comment.
I put 256 since I thought it was the default ignore_above value but I might be wrong:
https://www.elastic.co/guide/en/elasticsearch/reference/current/ignore-above.html
You are right that it can be larger however I'm not sure what the default value should be - it's significantly smaller than that of text - seems like Short.MAX_VALUE-1 (32766) is the max.
|
LGTM. |
| if (t.isInteger()) { | ||
| return Short.valueOf((short) 0); | ||
| } | ||
| if (t.isRational()) { | ||
| if (t == DATETIME || t.isRational()) { |
| 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), | ||
| SCALED_FLOAT( "scaled_float", JDBCType.DOUBLE, Integer.BYTES, 15, 25, false, true, true), | ||
| KEYWORD( "keyword", JDBCType.VARCHAR, Integer.MAX_VALUE, 256, 32766,false, false, true), |
There was a problem hiding this comment.
missing whitespace after 32766
| @@ -175,7 +175,7 @@ public static Integer metaSqlDataType(DataType t) { | |||
| } | |||
|
|
|||
| // https://github.com/elastic/elasticsearch/issues/30386 | |||
| // https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgettypeinfo-function?view=sql-server-2017 | |||
| // https://docs.microsoft.com/en-us/sql/odbc/reference/syntax/sqlgettypeinfo-function | |||
| public static Integer metaSqlDateTimeSub(DataType t) { | |||
| if (t == DATETIME) { | |||
There was a problem hiding this comment.
this was skipped but shouldn't be changed too?
There was a problem hiding this comment.
I don't think so since the type info indicates it should be just for datetime (timestamp) not DATE or TIME
| 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), | ||
| SCALED_FLOAT( "scaled_float", JDBCType.DOUBLE, Integer.BYTES, 15, 25, false, true, true), |
There was a problem hiding this comment.
is Integer.BYTES correct here? because the comment above says: precision is based on long
Fix #40357