Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL: Return Intervals in SQL format for CLI #37602

Merged
merged 4 commits into from
Jan 22, 2019
Merged

SQL: Return Intervals in SQL format for CLI #37602

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a3f949
Add separate CLI Mode
astefan Jan 17, 2019
5da9e7e
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Jan 18, 2019
3f5c3ef
Rename CliFormatter and have different formatting behavior for CLI and
astefan Jan 22, 2019
b617c0a
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Jan 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcHttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ Tuple<String, List<List<Object>>> nextPage(String cursor, RequestMeta meta) thro
}

boolean queryClose(String cursor) throws SQLException {
return httpClient.queryClose(cursor);
return httpClient.queryClose(cursor, Mode.JDBC);
}

InfoResponse serverInfo() throws SQLException {
Expand Down
95 changes: 55 additions & 40 deletions x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/SqlProtocolTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLIENT_IDS;
import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode;
import static org.elasticsearch.xpack.sql.proto.Mode.CLI;

public abstract class SqlProtocolTestCase extends ESRestTestCase {

Expand Down Expand Up @@ -79,57 +80,71 @@ public void testIPs() throws IOException {
}

public void testDateTimeIntervals() throws IOException {
assertQuery("SELECT INTERVAL '326' YEAR", "INTERVAL '326' YEAR", "interval_year", "P326Y", 7);
assertQuery("SELECT INTERVAL '50' MONTH", "INTERVAL '50' MONTH", "interval_month", "P50M", 7);
assertQuery("SELECT INTERVAL '520' DAY", "INTERVAL '520' DAY", "interval_day", "PT12480H", 23);
assertQuery("SELECT INTERVAL '163' HOUR", "INTERVAL '163' HOUR", "interval_hour", "PT163H", 23);
assertQuery("SELECT INTERVAL '163' MINUTE", "INTERVAL '163' MINUTE", "interval_minute", "PT2H43M", 23);
assertQuery("SELECT INTERVAL '223.16' SECOND", "INTERVAL '223.16' SECOND", "interval_second", "PT3M43.016S", 23);
assertQuery("SELECT INTERVAL '163-11' YEAR TO MONTH", "INTERVAL '163-11' YEAR TO MONTH", "interval_year_to_month", "P163Y11M", 7);
assertQuery("SELECT INTERVAL '163 12' DAY TO HOUR", "INTERVAL '163 12' DAY TO HOUR", "interval_day_to_hour", "PT3924H", 23);
assertQuery("SELECT INTERVAL '326' YEAR", "INTERVAL '326' YEAR", "interval_year", "P326Y", "+326-0", 7);
assertQuery("SELECT INTERVAL '50' MONTH", "INTERVAL '50' MONTH", "interval_month", "P50M", "+0-50", 7);
assertQuery("SELECT INTERVAL '520' DAY", "INTERVAL '520' DAY", "interval_day", "PT12480H", "+520 00:00:00.0", 23);
assertQuery("SELECT INTERVAL '163' HOUR", "INTERVAL '163' HOUR", "interval_hour", "PT163H", "+6 19:00:00.0", 23);
assertQuery("SELECT INTERVAL '163' MINUTE", "INTERVAL '163' MINUTE", "interval_minute", "PT2H43M", "+0 02:43:00.0", 23);
assertQuery("SELECT INTERVAL '223.16' SECOND", "INTERVAL '223.16' SECOND", "interval_second", "PT3M43.016S", "+0 00:03:43.16", 23);
assertQuery("SELECT INTERVAL '163-11' YEAR TO MONTH", "INTERVAL '163-11' YEAR TO MONTH", "interval_year_to_month", "P163Y11M",
"+163-11", 7);
assertQuery("SELECT INTERVAL '163 12' DAY TO HOUR", "INTERVAL '163 12' DAY TO HOUR", "interval_day_to_hour", "PT3924H",
"+163 12:00:00.0", 23);
assertQuery("SELECT INTERVAL '163 12:39' DAY TO MINUTE", "INTERVAL '163 12:39' DAY TO MINUTE", "interval_day_to_minute",
"PT3924H39M", 23);
"PT3924H39M", "+163 12:39:00.0", 23);
assertQuery("SELECT INTERVAL '163 12:39:59.163' DAY TO SECOND", "INTERVAL '163 12:39:59.163' DAY TO SECOND",
"interval_day_to_second", "PT3924H39M59.163S", 23);
"interval_day_to_second", "PT3924H39M59.163S", "+163 12:39:59.163", 23);
assertQuery("SELECT INTERVAL -'163 23:39:56.23' DAY TO SECOND", "INTERVAL -'163 23:39:56.23' DAY TO SECOND",
"interval_day_to_second", "PT-3935H-39M-56.023S", 23);
"interval_day_to_second", "PT-3935H-39M-56.023S", "-163 23:39:56.23", 23);
assertQuery("SELECT INTERVAL '163:39' HOUR TO MINUTE", "INTERVAL '163:39' HOUR TO MINUTE", "interval_hour_to_minute",
"PT163H39M", 23);
"PT163H39M", "+6 19:39:00.0", 23);
assertQuery("SELECT INTERVAL '163:39:59.163' HOUR TO SECOND", "INTERVAL '163:39:59.163' HOUR TO SECOND", "interval_hour_to_second",
"PT163H39M59.163S", 23);
"PT163H39M59.163S", "+6 19:39:59.163", 23);
assertQuery("SELECT INTERVAL '163:59.163' MINUTE TO SECOND", "INTERVAL '163:59.163' MINUTE TO SECOND", "interval_minute_to_second",
"PT2H43M59.163S", 23);
"PT2H43M59.163S", "+0 02:43:59.163", 23);
}

@SuppressWarnings({ "unchecked" })
private void assertQuery(String sql, String columnName, String columnType, Object columnValue, int displaySize) throws IOException {
private void assertQuery(String sql, String columnName, String columnType, Object columnValue, int displaySize)
throws IOException {
assertQuery(sql, columnName, columnType, columnValue, null, displaySize);
}

private void assertQuery(String sql, String columnName, String columnType, Object columnValue, Object cliColumnValue, int displaySize)
throws IOException {
for (Mode mode : Mode.values()) {
Map<String, Object> response = runSql(mode.toString(), sql);
List<Object> columns = (ArrayList<Object>) response.get("columns");
assertEquals(1, columns.size());
boolean isCliCheck = mode == CLI && cliColumnValue != null;
assertQuery(sql, columnName, columnType, isCliCheck ? cliColumnValue : columnValue, displaySize, mode);
}
}

@SuppressWarnings({ "unchecked" })
private void assertQuery(String sql, String columnName, String columnType, Object columnValue, int displaySize, Mode mode)
throws IOException {
Map<String, Object> response = runSql(mode.toString(), sql);
List<Object> columns = (ArrayList<Object>) response.get("columns");
assertEquals(1, columns.size());

Map<String, Object> column = (HashMap<String, Object>) columns.get(0);
assertEquals(columnName, column.get("name"));
assertEquals(columnType, column.get("type"));
if (mode != Mode.PLAIN) {
assertEquals(3, column.size());
assertEquals(displaySize, column.get("display_size"));
} else {
assertEquals(2, column.size());
}
List<Object> rows = (ArrayList<Object>) response.get("rows");
assertEquals(1, rows.size());
List<Object> row = (ArrayList<Object>) rows.get(0);
assertEquals(1, row.size());
Map<String, Object> column = (HashMap<String, Object>) columns.get(0);
assertEquals(columnName, column.get("name"));
assertEquals(columnType, column.get("type"));
if (Mode.isDriver(mode)) {
assertEquals(3, column.size());
assertEquals(displaySize, column.get("display_size"));
} else {
assertEquals(2, column.size());
}

List<Object> rows = (ArrayList<Object>) response.get("rows");
assertEquals(1, rows.size());
List<Object> row = (ArrayList<Object>) rows.get(0);
assertEquals(1, row.size());

// from xcontent we can get float or double, depending on the conversion
// method of the specific xcontent format implementation
if (columnValue instanceof Float && row.get(0) instanceof Double) {
assertEquals(columnValue, (float)((Number) row.get(0)).doubleValue());
} else {
assertEquals(columnValue, row.get(0));
}
// from xcontent we can get float or double, depending on the conversion
// method of the specific xcontent format implementation
if (columnValue instanceof Float && row.get(0) instanceof Double) {
assertEquals(columnValue, (float)((Number) row.get(0)).doubleValue());
} else {
assertEquals(columnValue, row.get(0));
}
}

Expand Down
5 changes: 3 additions & 2 deletions ...plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,10 @@ private void runSql(String sql) throws IOException {
String mode = Mode.PLAIN.toString();
if (clientType.equals(ClientType.JDBC.toString())) {
mode = Mode.JDBC.toString();
}
if (clientType.startsWith(ClientType.ODBC.toString())) {
} else if (clientType.startsWith(ClientType.ODBC.toString())) {
mode = Mode.ODBC.toString();
} else if (clientType.equals(ClientType.CLI.toString())) {
mode = Mode.CLI.toString();
}

runSql(mode, clientType, sql);
Expand Down
10 changes: 3 additions & 7 deletions .../plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/CliFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.xpack.sql.proto.ColumnInfo;
import org.elasticsearch.xpack.sql.proto.StringUtils;

import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;

/**
* Formats {@link SqlQueryResponse} for the CLI. {@linkplain Writeable} so
Expand Down Expand Up @@ -43,9 +43,7 @@ public CliFormatter(List<ColumnInfo> columns, List<List<Object>> rows) {
// 2. Expand columns to fit the largest value
for (List<Object> row : rows) {
for (int i = 0; i < width.length; i++) {
// TODO are we sure toString is correct here? What about dates that come back as longs.
// Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3081
width[i] = Math.max(width[i], StringUtils.toString(row.get(i)).length());
width[i] = Math.max(width[i], Objects.toString(row.get(i)).length());
}
}
}
Expand Down Expand Up @@ -116,9 +114,7 @@ private String formatWithoutHeader(StringBuilder sb, List<List<Object>> rows) {
if (i > 0) {
sb.append('|');
}
// TODO are we sure toString is correct here? What about dates that come back as longs.
// Tracked by https://github.com/elastic/x-pack-elasticsearch/issues/3081
String string = StringUtils.toString(row.get(i));
String string = Objects.toString(row.get(i));
if (string.length() <= width[i]) {
// Pad
sb.append(string);
Expand Down
9 changes: 8 additions & 1 deletion ...gin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

import static java.util.Collections.unmodifiableList;
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.CURSOR;
import static org.elasticsearch.xpack.sql.proto.Mode.CLI;

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

public SqlQueryResponse() {
}
Expand Down Expand Up @@ -173,7 +175,12 @@ public static XContentBuilder value(XContentBuilder builder, Mode mode, Object v
ZonedDateTime zdt = (ZonedDateTime) value;
// use the ISO format
builder.value(StringUtils.toString(zdt));
} else {
} else if (mode == CLI && value != null && value.getClass().getSuperclass().getSimpleName().equals(INTERVAL_CLASS_NAME)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use the isAssignableFrom() and avoid the string check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but Interval is not visible in the action project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, thx!

// use the SQL format for intervals when sending back the response for CLI
// all other clients will receive ISO 8601 formatted intervals
builder.value(value.toString());
}
else {
builder.value(value);
}
return builder;
Expand Down
2 changes: 1 addition & 1 deletion ...l/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public void testClearCursorRequestParser() throws IOException {

request = generateRequest("{\"cursor\" : \"whatever\", \"client_id\" : \"CLI\"}",
SqlClearCursorRequest::fromXContent);
assertEquals("cli", request.clientId());
assertNull(request.clientId());
assertEquals(Mode.PLAIN, request.mode());
assertEquals("whatever", request.getCursor());

Expand Down
3 changes: 2 additions & 1 deletion .../sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/command/ServerQueryCliCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.elasticsearch.xpack.sql.cli.CliTerminal;
import org.elasticsearch.xpack.sql.client.HttpClient;
import org.elasticsearch.xpack.sql.client.JreHttpUrlConnection;
import org.elasticsearch.xpack.sql.proto.Mode;
import org.elasticsearch.xpack.sql.proto.SqlQueryResponse;

import java.sql.SQLException;
Expand Down Expand Up @@ -46,7 +47,7 @@ protected boolean doHandle(CliTerminal terminal, CliSession cliSession, String l
}
if (response != null) {
try {
cliClient.queryClose(response.cursor());
cliClient.queryClose(response.cursor(), Mode.CLI);
} catch (SQLException ex) {
terminal.error("Could not close cursor", ex.getMessage());
}
Expand Down
5 changes: 3 additions & 2 deletions ...cli/src/test/java/org/elasticsearch/xpack/sql/cli/command/ServerQueryCliCommandTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.elasticsearch.xpack.sql.cli.TestTerminal;
import org.elasticsearch.xpack.sql.client.HttpClient;
import org.elasticsearch.xpack.sql.proto.ColumnInfo;
import org.elasticsearch.xpack.sql.proto.Mode;
import org.elasticsearch.xpack.sql.proto.SqlQueryResponse;

import java.sql.SQLException;
Expand Down Expand Up @@ -93,14 +94,14 @@ public void testCursorCleanupOnError() throws Exception {
cliSession.setFetchSize(15);
when(client.queryInit("test query", 15)).thenReturn(fakeResponse("my_cursor1", true, "first"));
when(client.nextPage("my_cursor1")).thenThrow(new SQLException("test exception"));
when(client.queryClose("my_cursor1")).thenReturn(true);
when(client.queryClose("my_cursor1", Mode.CLI)).thenReturn(true);
ServerQueryCliCommand cliCommand = new ServerQueryCliCommand();
assertTrue(cliCommand.handle(testTerminal, cliSession, "test query"));
assertEquals(" field \n---------------\nfirst \n" +
"<b>Bad request [</b><i>test exception</i><b>]</b>\n", testTerminal.toString());
verify(client, times(1)).queryInit(eq("test query"), eq(15));
verify(client, times(1)).nextPage(any());
verify(client, times(1)).queryClose(eq("my_cursor1"));
verify(client, times(1)).queryClose(eq("my_cursor1"), eq(Mode.CLI));
verifyNoMoreInteractions(client);
}

Expand Down
14 changes: 6 additions & 8 deletions ...ck/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@
import java.util.Collections;
import java.util.function.Function;

import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI;

/**
* A specialized high-level REST client with support for SQL-related functions.
* Similar to JDBC and the underlying HTTP connection, this class is not thread-safe
Expand Down Expand Up @@ -65,10 +63,10 @@ public MainResponse serverInfo() throws SQLException {

public SqlQueryResponse queryInit(String query, int fetchSize) throws SQLException {
// TODO allow customizing the time zone - this is what session set/reset/get should be about
// method called only from CLI. "client_id" is set to "cli"
// method called only from CLI
SqlQueryRequest sqlRequest = new SqlQueryRequest(query, Collections.emptyList(), null, ZoneId.of("Z"),
fetchSize, TimeValue.timeValueMillis(cfg.queryTimeout()), TimeValue.timeValueMillis(cfg.pageTimeout()),
new RequestInfo(Mode.PLAIN, CLI));
new RequestInfo(Mode.CLI));
return query(sqlRequest);
}

Expand All @@ -77,15 +75,15 @@ public SqlQueryResponse query(SqlQueryRequest sqlRequest) throws SQLException {
}

public SqlQueryResponse nextPage(String cursor) throws SQLException {
// method called only from CLI. "client_id" is set to "cli"
// method called only from CLI
SqlQueryRequest sqlRequest = new SqlQueryRequest(cursor, TimeValue.timeValueMillis(cfg.queryTimeout()),
TimeValue.timeValueMillis(cfg.pageTimeout()), new RequestInfo(Mode.PLAIN, CLI));
TimeValue.timeValueMillis(cfg.pageTimeout()), new RequestInfo(Mode.CLI));
return post(Protocol.SQL_QUERY_REST_ENDPOINT, sqlRequest, SqlQueryResponse::fromXContent);
}

public boolean queryClose(String cursor) throws SQLException {
public boolean queryClose(String cursor, Mode mode) throws SQLException {
SqlClearCursorResponse response = post(Protocol.CLEAR_CURSOR_REST_ENDPOINT,
new SqlClearCursorRequest(cursor, new RequestInfo(Mode.PLAIN)),
new SqlClearCursorRequest(cursor, new RequestInfo(mode)),
SqlClearCursorResponse::fromXContent);
return response.isSucceeded();
}
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
* SQL protocol mode
*/
public enum Mode {
CLI,
PLAIN,
JDBC,
ODBC;
Expand Down
2 changes: 0 additions & 2 deletions x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import java.util.Set;

public class RequestInfo {
public static final String CLI = "cli";
private static final String CANVAS = "canvas";
public static final String ODBC_32 = "odbc32";
private static final String ODBC_64 = "odbc64";
Expand All @@ -22,7 +21,6 @@ public class RequestInfo {

static {
Set<String> clientIds = new HashSet<>(4);
clientIds.add(CLI);
clientIds.add(CANVAS);
clientIds.add(ODBC_32);
clientIds.add(ODBC_64);
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ public class SqlPlugin extends Plugin implements ActionPlugin {
}
break;
case PLAIN:
case CLI:
if (licenseState.isSqlAllowed() == false) {
throw LicenseUtils.newComplianceException(XPackField.SQL);
}
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/action/CliFormatterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
import static org.hamcrest.Matchers.arrayWithSize;

public class CliFormatterTests extends ESTestCase {
private final SqlQueryResponse firstResponse = new SqlQueryResponse("", Mode.PLAIN,
private final SqlQueryResponse firstResponse = new SqlQueryResponse("", Mode.CLI,
Arrays.asList(
new ColumnInfo("", "foo", "string", 0),
new ColumnInfo("", "bar", "long", 15),
Expand Down