Skip to content

Commit e80c5f3

Browse files
astefanastefan
committed
SQL: Return Intervals in SQL format for CLI (#37602)
* Add separate CLI Mode * Use the correct Mode for cursor close requests * Renamed CliFormatter and have different formatting behavior for CLI and "text" format. (cherry picked from commit 7507af2)
1 parent 6a0f149 commit e80c5f3

File tree

18 files changed

+191
-124
lines changed

18 files changed

+191
-124
lines changed

‎x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcHttpClient.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ Tuple<String, List<List<Object>>> nextPage(String cursor, RequestMeta meta) thro
6868
}
6969

7070
boolean queryClose(String cursor) throws SQLException {
71-
return httpClient.queryClose(cursor);
71+
return httpClient.queryClose(cursor, Mode.JDBC);
7272
}
7373

7474
InfoResponse serverInfo() throws SQLException {

‎x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/SqlProtocolTestCase.java

Lines changed: 55 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import static org.elasticsearch.xpack.sql.proto.Protocol.SQL_QUERY_REST_ENDPOINT;
3232
import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLIENT_IDS;
3333
import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode;
34+
import static org.elasticsearch.xpack.sql.proto.Mode.CLI;
3435

3536
public abstract class SqlProtocolTestCase extends ESRestTestCase {
3637

@@ -80,57 +81,71 @@ public void testIPs() throws IOException {
8081
}
8182

8283
public void testDateTimeIntervals() throws IOException {
83-
assertQuery("SELECT INTERVAL '326' YEAR", "INTERVAL '326' YEAR", "interval_year", "P326Y", 7);
84-
assertQuery("SELECT INTERVAL '50' MONTH", "INTERVAL '50' MONTH", "interval_month", "P50M", 7);
85-
assertQuery("SELECT INTERVAL '520' DAY", "INTERVAL '520' DAY", "interval_day", "PT12480H", 23);
86-
assertQuery("SELECT INTERVAL '163' HOUR", "INTERVAL '163' HOUR", "interval_hour", "PT163H", 23);
87-
assertQuery("SELECT INTERVAL '163' MINUTE", "INTERVAL '163' MINUTE", "interval_minute", "PT2H43M", 23);
88-
assertQuery("SELECT INTERVAL '223.16' SECOND", "INTERVAL '223.16' SECOND", "interval_second", "PT3M43.016S", 23);
89-
assertQuery("SELECT INTERVAL '163-11' YEAR TO MONTH", "INTERVAL '163-11' YEAR TO MONTH", "interval_year_to_month", "P163Y11M", 7);
90-
assertQuery("SELECT INTERVAL '163 12' DAY TO HOUR", "INTERVAL '163 12' DAY TO HOUR", "interval_day_to_hour", "PT3924H", 23);
84+
assertQuery("SELECT INTERVAL '326' YEAR", "INTERVAL '326' YEAR", "interval_year", "P326Y", "+326-0", 7);
85+
assertQuery("SELECT INTERVAL '50' MONTH", "INTERVAL '50' MONTH", "interval_month", "P50M", "+0-50", 7);
86+
assertQuery("SELECT INTERVAL '520' DAY", "INTERVAL '520' DAY", "interval_day", "PT12480H", "+520 00:00:00.0", 23);
87+
assertQuery("SELECT INTERVAL '163' HOUR", "INTERVAL '163' HOUR", "interval_hour", "PT163H", "+6 19:00:00.0", 23);
88+
assertQuery("SELECT INTERVAL '163' MINUTE", "INTERVAL '163' MINUTE", "interval_minute", "PT2H43M", "+0 02:43:00.0", 23);
89+
assertQuery("SELECT INTERVAL '223.16' SECOND", "INTERVAL '223.16' SECOND", "interval_second", "PT3M43.016S", "+0 00:03:43.16", 23);
90+
assertQuery("SELECT INTERVAL '163-11' YEAR TO MONTH", "INTERVAL '163-11' YEAR TO MONTH", "interval_year_to_month", "P163Y11M",
91+
"+163-11", 7);
92+
assertQuery("SELECT INTERVAL '163 12' DAY TO HOUR", "INTERVAL '163 12' DAY TO HOUR", "interval_day_to_hour", "PT3924H",
93+
"+163 12:00:00.0", 23);
9194
assertQuery("SELECT INTERVAL '163 12:39' DAY TO MINUTE", "INTERVAL '163 12:39' DAY TO MINUTE", "interval_day_to_minute",
92-
"PT3924H39M", 23);
95+
"PT3924H39M", "+163 12:39:00.0", 23);
9396
assertQuery("SELECT INTERVAL '163 12:39:59.163' DAY TO SECOND", "INTERVAL '163 12:39:59.163' DAY TO SECOND",
94-
"interval_day_to_second", "PT3924H39M59.163S", 23);
97+
"interval_day_to_second", "PT3924H39M59.163S", "+163 12:39:59.163", 23);
9598
assertQuery("SELECT INTERVAL -'163 23:39:56.23' DAY TO SECOND", "INTERVAL -'163 23:39:56.23' DAY TO SECOND",
96-
"interval_day_to_second", "PT-3935H-39M-56.023S", 23);
99+
"interval_day_to_second", "PT-3935H-39M-56.023S", "-163 23:39:56.23", 23);
97100
assertQuery("SELECT INTERVAL '163:39' HOUR TO MINUTE", "INTERVAL '163:39' HOUR TO MINUTE", "interval_hour_to_minute",
98-
"PT163H39M", 23);
101+
"PT163H39M", "+6 19:39:00.0", 23);
99102
assertQuery("SELECT INTERVAL '163:39:59.163' HOUR TO SECOND", "INTERVAL '163:39:59.163' HOUR TO SECOND", "interval_hour_to_second",
100-
"PT163H39M59.163S", 23);
103+
"PT163H39M59.163S", "+6 19:39:59.163", 23);
101104
assertQuery("SELECT INTERVAL '163:59.163' MINUTE TO SECOND", "INTERVAL '163:59.163' MINUTE TO SECOND", "interval_minute_to_second",
102-
"PT2H43M59.163S", 23);
105+
"PT2H43M59.163S", "+0 02:43:59.163", 23);
103106
}
104107

105-
@SuppressWarnings({ "unchecked" })
106-
private void assertQuery(String sql, String columnName, String columnType, Object columnValue, int displaySize) throws IOException {
108+
private void assertQuery(String sql, String columnName, String columnType, Object columnValue, int displaySize)
109+
throws IOException {
110+
assertQuery(sql, columnName, columnType, columnValue, null, displaySize);
111+
}
112+
113+
private void assertQuery(String sql, String columnName, String columnType, Object columnValue, Object cliColumnValue, int displaySize)
114+
throws IOException {
107115
for (Mode mode : Mode.values()) {
108-
Map<String, Object> response = runSql(mode.toString(), sql);
109-
List<Object> columns = (ArrayList<Object>) response.get("columns");
110-
assertEquals(1, columns.size());
116+
boolean isCliCheck = mode == CLI && cliColumnValue != null;
117+
assertQuery(sql, columnName, columnType, isCliCheck ? cliColumnValue : columnValue, displaySize, mode);
118+
}
119+
}
120+
121+
@SuppressWarnings({ "unchecked" })
122+
private void assertQuery(String sql, String columnName, String columnType, Object columnValue, int displaySize, Mode mode)
123+
throws IOException {
124+
Map<String, Object> response = runSql(mode.toString(), sql);
125+
List<Object> columns = (ArrayList<Object>) response.get("columns");
126+
assertEquals(1, columns.size());
111127

112-
Map<String, Object> column = (HashMap<String, Object>) columns.get(0);
113-
assertEquals(columnName, column.get("name"));
114-
assertEquals(columnType, column.get("type"));
115-
if (mode != Mode.PLAIN) {
116-
assertEquals(3, column.size());
117-
assertEquals(displaySize, column.get("display_size"));
118-
} else {
119-
assertEquals(2, column.size());
120-
}
121-
122-
List<Object> rows = (ArrayList<Object>) response.get("rows");
123-
assertEquals(1, rows.size());
124-
List<Object> row = (ArrayList<Object>) rows.get(0);
125-
assertEquals(1, row.size());
128+
Map<String, Object> column = (HashMap<String, Object>) columns.get(0);
129+
assertEquals(columnName, column.get("name"));
130+
assertEquals(columnType, column.get("type"));
131+
if (Mode.isDriver(mode)) {
132+
assertEquals(3, column.size());
133+
assertEquals(displaySize, column.get("display_size"));
134+
} else {
135+
assertEquals(2, column.size());
136+
}
137+
138+
List<Object> rows = (ArrayList<Object>) response.get("rows");
139+
assertEquals(1, rows.size());
140+
List<Object> row = (ArrayList<Object>) rows.get(0);
141+
assertEquals(1, row.size());
126142

127-
// from xcontent we can get float or double, depending on the conversion
128-
// method of the specific xcontent format implementation
129-
if (columnValue instanceof Float && row.get(0) instanceof Double) {
130-
assertEquals(columnValue, (float)((Number) row.get(0)).doubleValue());
131-
} else {
132-
assertEquals(columnValue, row.get(0));
133-
}
143+
// from xcontent we can get float or double, depending on the conversion
144+
// method of the specific xcontent format implementation
145+
if (columnValue instanceof Float && row.get(0) instanceof Double) {
146+
assertEquals(columnValue, (float)((Number) row.get(0)).doubleValue());
147+
} else {
148+
assertEquals(columnValue, row.get(0));
134149
}
135150
}
136151

‎x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcTestUtils.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
package org.elasticsearch.xpack.sql.qa.jdbc;
77

88
import org.apache.logging.log4j.Logger;
9-
import org.elasticsearch.xpack.sql.action.CliFormatter;
9+
import org.elasticsearch.xpack.sql.action.BasicFormatter;
1010
import org.elasticsearch.xpack.sql.proto.ColumnInfo;
1111
import org.elasticsearch.xpack.sql.proto.StringUtils;
1212

@@ -19,6 +19,8 @@
1919
import java.util.ArrayList;
2020
import java.util.List;
2121

22+
import static org.elasticsearch.xpack.sql.action.BasicFormatter.FormatOption.CLI;
23+
2224
public abstract class JdbcTestUtils {
2325

2426
public static final String SQL_TRACE = "org.elasticsearch.xpack.sql:TRACE";
@@ -131,7 +133,7 @@ public static void logLikeCLI(ResultSet rs, Logger logger) throws SQLException {
131133
data.add(entry);
132134
}
133135

134-
CliFormatter formatter = new CliFormatter(cols, data);
136+
BasicFormatter formatter = new BasicFormatter(cols, data, CLI);
135137
logger.info("\n" + formatter.formatWithHeader(cols, data));
136138
}
137139

‎x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,9 +258,10 @@ private void runSql(String sql) throws IOException {
258258
String mode = Mode.PLAIN.toString();
259259
if (clientType.equals(ClientType.JDBC.toString())) {
260260
mode = Mode.JDBC.toString();
261-
}
262-
if (clientType.startsWith(ClientType.ODBC.toString())) {
261+
} else if (clientType.startsWith(ClientType.ODBC.toString())) {
263262
mode = Mode.ODBC.toString();
263+
} else if (clientType.equals(ClientType.CLI.toString())) {
264+
mode = Mode.CLI.toString();
264265
}
265266

266267
runSql(mode, clientType, sql);

‎x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/CliFormatter.java renamed to ‎x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/BasicFormatter.java

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,26 +14,46 @@
1414
import java.io.IOException;
1515
import java.util.Arrays;
1616
import java.util.List;
17+
import java.util.Objects;
18+
import java.util.function.Function;
1719

1820
/**
19-
* Formats {@link SqlQueryResponse} for the CLI. {@linkplain Writeable} so
21+
* Formats {@link SqlQueryResponse} for the CLI and for the TEXT format. {@linkplain Writeable} so
2022
* that its state can be saved between pages of results.
2123
*/
22-
public class CliFormatter implements Writeable {
24+
public class BasicFormatter implements Writeable {
2325
/**
2426
* The minimum width for any column in the formatted results.
2527
*/
2628
private static final int MIN_COLUMN_WIDTH = 15;
2729

2830
private int[] width;
31+
32+
public enum FormatOption {
33+
CLI(Objects::toString),
34+
TEXT(StringUtils::toString);
35+
36+
private final Function<Object, String> apply;
37+
38+
FormatOption(Function<Object, String> apply) {
39+
this.apply = l -> l == null ? null : apply.apply(l);
40+
}
41+
42+
public final String apply(Object l) {
43+
return apply.apply(l);
44+
}
45+
}
46+
47+
private final FormatOption formatOption;
2948

3049
/**
31-
* Create a new {@linkplain CliFormatter} for formatting responses similar
50+
* Create a new {@linkplain BasicFormatter} for formatting responses similar
3251
* to the provided columns and rows.
3352
*/
34-
public CliFormatter(List<ColumnInfo> columns, List<List<Object>> rows) {
53+
public BasicFormatter(List<ColumnInfo> columns, List<List<Object>> rows, FormatOption formatOption) {
3554
// Figure out the column widths:
3655
// 1. Start with the widths of the column names
56+
this.formatOption = formatOption;
3757
width = new int[columns.size()];
3858
for (int i = 0; i < width.length; i++) {
3959
// TODO read the width from the data type?
@@ -43,24 +63,24 @@ public CliFormatter(List<ColumnInfo> columns, List<List<Object>> rows) {
4363
// 2. Expand columns to fit the largest value
4464
for (List<Object> row : rows) {
4565
for (int i = 0; i < width.length; i++) {
46-
// TODO are we sure toString is correct here? What about dates that come back as longs.
47-
// Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3081
48-
width[i] = Math.max(width[i], StringUtils.toString(row.get(i)).length());
66+
width[i] = Math.max(width[i], formatOption.apply(row.get(i)).length());
4967
}
5068
}
5169
}
5270

53-
public CliFormatter(StreamInput in) throws IOException {
71+
public BasicFormatter(StreamInput in) throws IOException {
5472
width = in.readIntArray();
73+
formatOption = in.readEnum(FormatOption.class);
5574
}
5675

5776
@Override
5877
public void writeTo(StreamOutput out) throws IOException {
5978
out.writeIntArray(width);
79+
out.writeEnum(formatOption);
6080
}
61-
81+
6282
/**
63-
* Format the provided {@linkplain SqlQueryResponse} for the CLI
83+
* Format the provided {@linkplain SqlQueryResponse} for the set format
6484
* including the header lines.
6585
*/
6686
public String formatWithHeader(List<ColumnInfo> columns, List<List<Object>> rows) {
@@ -103,7 +123,7 @@ public String formatWithHeader(List<ColumnInfo> columns, List<List<Object>> rows
103123
}
104124

105125
/**
106-
* Format the provided {@linkplain SqlQueryResponse} for the CLI
126+
* Format the provided {@linkplain SqlQueryResponse} for the set format
107127
* without the header lines.
108128
*/
109129
public String formatWithoutHeader(List<List<Object>> rows) {
@@ -116,9 +136,7 @@ private String formatWithoutHeader(StringBuilder sb, List<List<Object>> rows) {
116136
if (i > 0) {
117137
sb.append('|');
118138
}
119-
// TODO are we sure toString is correct here? What about dates that come back as longs.
120-
// Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3081
121-
String string = StringUtils.toString(row.get(i));
139+
String string = formatOption.apply(row.get(i));
122140
if (string.length() <= width[i]) {
123141
// Pad
124142
sb.append(string);
@@ -159,12 +177,12 @@ public boolean equals(Object o) {
159177
if (o == null || getClass() != o.getClass()) {
160178
return false;
161179
}
162-
CliFormatter that = (CliFormatter) o;
163-
return Arrays.equals(width, that.width);
180+
BasicFormatter that = (BasicFormatter) o;
181+
return Arrays.equals(width, that.width) && formatOption == that.formatOption;
164182
}
165183

166184
@Override
167185
public int hashCode() {
168-
return Arrays.hashCode(width);
186+
return Objects.hash(width, formatOption);
169187
}
170188
}

‎x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
import static java.util.Collections.unmodifiableList;
2626
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.CURSOR;
27+
import static org.elasticsearch.xpack.sql.proto.Mode.CLI;
2728

2829
/**
2930
* Response to perform an sql query
@@ -36,6 +37,7 @@ public class SqlQueryResponse extends ActionResponse implements ToXContentObject
3637
private List<ColumnInfo> columns;
3738
// TODO investigate reusing Page here - it probably is much more efficient
3839
private List<List<Object>> rows;
40+
private static final String INTERVAL_CLASS_NAME = "Interval";
3941

4042
public SqlQueryResponse() {
4143
}
@@ -173,7 +175,12 @@ public static XContentBuilder value(XContentBuilder builder, Mode mode, Object v
173175
ZonedDateTime zdt = (ZonedDateTime) value;
174176
// use the ISO format
175177
builder.value(StringUtils.toString(zdt));
176-
} else {
178+
} else if (mode == CLI && value != null && value.getClass().getSuperclass().getSimpleName().equals(INTERVAL_CLASS_NAME)) {
179+
// use the SQL format for intervals when sending back the response for CLI
180+
// all other clients will receive ISO 8601 formatted intervals
181+
builder.value(value.toString());
182+
}
183+
else {
177184
builder.value(value);
178185
}
179186
return builder;

‎x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public void testClearCursorRequestParser() throws IOException {
6868

6969
request = generateRequest("{\"cursor\" : \"whatever\", \"client_id\" : \"CLI\"}",
7070
SqlClearCursorRequest::fromXContent);
71-
assertEquals("cli", request.clientId());
71+
assertNull(request.clientId());
7272
assertEquals(Mode.PLAIN, request.mode());
7373
assertEquals("whatever", request.getCursor());
7474

‎x-pack/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/command/ServerQueryCliCommand.java

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,26 +5,29 @@
55
*/
66
package org.elasticsearch.xpack.sql.cli.command;
77

8-
import org.elasticsearch.xpack.sql.action.CliFormatter;
8+
import org.elasticsearch.xpack.sql.action.BasicFormatter;
99
import org.elasticsearch.xpack.sql.cli.CliTerminal;
1010
import org.elasticsearch.xpack.sql.client.HttpClient;
1111
import org.elasticsearch.xpack.sql.client.JreHttpUrlConnection;
12+
import org.elasticsearch.xpack.sql.proto.Mode;
1213
import org.elasticsearch.xpack.sql.proto.SqlQueryResponse;
1314

1415
import java.sql.SQLException;
1516

17+
import static org.elasticsearch.xpack.sql.action.BasicFormatter.FormatOption.CLI;
18+
1619
public class ServerQueryCliCommand extends AbstractServerCliCommand {
1720

1821
@Override
1922
protected boolean doHandle(CliTerminal terminal, CliSession cliSession, String line) {
2023
SqlQueryResponse response = null;
2124
HttpClient cliClient = cliSession.getClient();
22-
CliFormatter cliFormatter;
25+
BasicFormatter formatter;
2326
String data;
2427
try {
2528
response = cliClient.queryInit(line, cliSession.getFetchSize());
26-
cliFormatter = new CliFormatter(response.columns(), response.rows());
27-
data = cliFormatter.formatWithHeader(response.columns(), response.rows());
29+
formatter = new BasicFormatter(response.columns(), response.rows(), CLI);
30+
data = formatter.formatWithHeader(response.columns(), response.rows());
2831
while (true) {
2932
handleText(terminal, data);
3033
if (response.cursor().isEmpty()) {
@@ -36,7 +39,7 @@ protected boolean doHandle(CliTerminal terminal, CliSession cliSession, String l
3639
terminal.println(cliSession.getFetchSeparator());
3740
}
3841
response = cliSession.getClient().nextPage(response.cursor());
39-
data = cliFormatter.formatWithoutHeader(response.rows());
42+
data = formatter.formatWithoutHeader(response.rows());
4043
}
4144
} catch (SQLException e) {
4245
if (JreHttpUrlConnection.SQL_STATE_BAD_SERVER.equals(e.getSQLState())) {
@@ -46,7 +49,7 @@ protected boolean doHandle(CliTerminal terminal, CliSession cliSession, String l
4649
}
4750
if (response != null) {
4851
try {
49-
cliClient.queryClose(response.cursor());
52+
cliClient.queryClose(response.cursor(), Mode.CLI);
5053
} catch (SQLException ex) {
5154
terminal.error("Could not close cursor", ex.getMessage());
5255
}

0 commit comments

Comments
 (0)