Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
6 changes: 6 additions & 0 deletions docs/changelog/83795.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 83795
summary: Add leniency option to SQL CLI
area: SQL
type: enhancement
issues:
- 67436
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,14 @@ public void testSelectWithWhere() throws IOException {
assertThat(readLine(), RegexMatcher.matches("\\s*2\\s*\\|\\s*test_value2\\s*"));
assertEquals("", readLine());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I would create a test class just for this command. And, apart from the test you have below, I'd test the false scenario as well (the one that generates an error).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

public void testLenientCommand() throws IOException {
index("test", body -> body.field("name", "foo").field("tags", new String[] { "bar", "bar" }));
assertThat(command("lenient = true"), containsString("true"));
assertThat(command("SELECT * FROM test"), RegexMatcher.matches("\\s*name\\s*\\|\\s*tags\\s*"));
assertThat(readLine(), containsString("----------"));
assertThat(readLine(), RegexMatcher.matches("\\s*foo\\s*\\|\\s*bar\\s*"));
assertEquals("", readLine());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.xpack.sql.cli.command.CliSession;
import org.elasticsearch.xpack.sql.cli.command.FetchSeparatorCliCommand;
import org.elasticsearch.xpack.sql.cli.command.FetchSizeCliCommand;
import org.elasticsearch.xpack.sql.cli.command.LenientCliCommand;
import org.elasticsearch.xpack.sql.cli.command.PrintLogoCommand;
import org.elasticsearch.xpack.sql.cli.command.ServerInfoCliCommand;
import org.elasticsearch.xpack.sql.cli.command.ServerQueryCliCommand;
Expand Down Expand Up @@ -102,6 +103,7 @@ public Cli(CliTerminal cliTerminal) {
.withRequiredArg()
.ofType(Boolean.class)
.defaultsTo(Boolean.parseBoolean(System.getProperty("cli.check", "true")));

this.connectionString = parser.nonOptions("uri");
}

Expand All @@ -128,6 +130,7 @@ private void execute(String uri, boolean debug, boolean binary, String keystoreL
new PrintLogoCommand(),
new ClearScreenCliCommand(),
new FetchSizeCliCommand(),
new LenientCliCommand(),
new FetchSeparatorCliCommand(),
new ServerInfoCliCommand(),
new ServerQueryCliCommand()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public class CliSession {
private String fetchSeparator = "";
private boolean debug;
private boolean binary;
private boolean lenient = CoreProtocol.FIELD_MULTI_VALUE_LENIENCY;
Copy link
Contributor

Choose a reason for hiding this comment

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

There are more and more commands and configuration settings to set. I think it's worth considering taking all these out in a separate class, like SqlConfiguration one for example, and the CliSession to contain a reference to this configuration instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it makes sense. Just wondering if it makes sense to have a separate PR for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


public CliSession(HttpClient httpClient) {
this.httpClient = httpClient;
Expand Down Expand Up @@ -68,6 +69,14 @@ public boolean isBinary() {
return binary;
}

public boolean isLenient() {
return lenient;
}

public void setLenient(boolean lenient) {
this.lenient = lenient;
}

public void checkConnection() throws ClientException {
MainResponse response;
try {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
package org.elasticsearch.xpack.sql.cli.command;

import org.elasticsearch.xpack.sql.cli.CliTerminal;

import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
* lenient command, enables/disables fields multi-value leniency.
* ie. with lenient = true, in case of array values, return the first value, with no guarantee of consistent results.
*
*/
public class LenientCliCommand extends AbstractCliCommand {

public LenientCliCommand() {
super(Pattern.compile("lenient *= *(.+)", Pattern.CASE_INSENSITIVE));
}

@Override
protected boolean doHandle(CliTerminal terminal, CliSession cliSession, Matcher m, String line) {
cliSession.setLenient(Boolean.parseBoolean(m.group(1)));
terminal.line().text("lenient set to ").em(Boolean.toString(cliSession.isLenient())).end();
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ protected boolean doHandle(CliTerminal terminal, CliSession cliSession, String l
SimpleFormatter formatter;
String data;
try {
response = cliClient.basicQuery(line, cliSession.getFetchSize());
response = cliClient.basicQuery(line, cliSession.getFetchSize(), cliSession.isLenient());
formatter = new SimpleFormatter(response.columns(), response.rows(), CLI);
data = formatter.formatWithHeader(response.columns(), response.rows());
while (true) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ public void testExceptionHandling() throws Exception {
TestTerminal testTerminal = new TestTerminal();
HttpClient client = mock(HttpClient.class);
CliSession cliSession = new CliSession(client);
when(client.basicQuery("blah", 1000)).thenThrow(new SQLException("test exception"));
when(client.basicQuery("blah", 1000, false)).thenThrow(new SQLException("test exception"));
ServerQueryCliCommand cliCommand = new ServerQueryCliCommand();
assertTrue(cliCommand.handle(testTerminal, cliSession, "blah"));
assertEquals("<b>Bad request [</b><i>test exception</i><b>]</b>\n", testTerminal.toString());
verify(client, times(1)).basicQuery(eq("blah"), eq(1000));
verify(client, times(1)).basicQuery(eq("blah"), eq(1000), eq(false));
verifyNoMoreInteractions(client);
}

Expand All @@ -45,15 +45,15 @@ public void testOnePageQuery() throws Exception {
HttpClient client = mock(HttpClient.class);
CliSession cliSession = new CliSession(client);
cliSession.setFetchSize(10);
when(client.basicQuery("test query", 10)).thenReturn(fakeResponse("", true, "foo"));
when(client.basicQuery("test query", 10, false)).thenReturn(fakeResponse("", true, "foo"));
ServerQueryCliCommand cliCommand = new ServerQueryCliCommand();
assertTrue(cliCommand.handle(testTerminal, cliSession, "test query"));
assertEquals("""
field \s
---------------
foo \s
<flush/>""", testTerminal.toString());
verify(client, times(1)).basicQuery(eq("test query"), eq(10));
verify(client, times(1)).basicQuery(eq("test query"), eq(10), eq(false));
verifyNoMoreInteractions(client);
}

Expand All @@ -62,7 +62,7 @@ public void testThreePageQuery() throws Exception {
HttpClient client = mock(HttpClient.class);
CliSession cliSession = new CliSession(client);
cliSession.setFetchSize(10);
when(client.basicQuery("test query", 10)).thenReturn(fakeResponse("my_cursor1", true, "first"));
when(client.basicQuery("test query", 10, false)).thenReturn(fakeResponse("my_cursor1", true, "first"));
when(client.nextPage("my_cursor1")).thenReturn(fakeResponse("my_cursor2", false, "second"));
when(client.nextPage("my_cursor2")).thenReturn(fakeResponse("", false, "third"));
ServerQueryCliCommand cliCommand = new ServerQueryCliCommand();
Expand All @@ -74,7 +74,7 @@ public void testThreePageQuery() throws Exception {
second \s
third \s
<flush/>""", testTerminal.toString());
verify(client, times(1)).basicQuery(eq("test query"), eq(10));
verify(client, times(1)).basicQuery(eq("test query"), eq(10), eq(false));
verify(client, times(2)).nextPage(any());
verifyNoMoreInteractions(client);
}
Expand All @@ -86,7 +86,7 @@ public void testTwoPageQueryWithSeparator() throws Exception {
cliSession.setFetchSize(15);
// Set a separator
cliSession.setFetchSeparator("-----");
when(client.basicQuery("test query", 15)).thenReturn(fakeResponse("my_cursor1", true, "first"));
when(client.basicQuery("test query", 15, false)).thenReturn(fakeResponse("my_cursor1", true, "first"));
when(client.nextPage("my_cursor1")).thenReturn(fakeResponse("", false, "second"));
ServerQueryCliCommand cliCommand = new ServerQueryCliCommand();
assertTrue(cliCommand.handle(testTerminal, cliSession, "test query"));
Expand All @@ -97,7 +97,7 @@ public void testTwoPageQueryWithSeparator() throws Exception {
-----
second \s
<flush/>""", testTerminal.toString());
verify(client, times(1)).basicQuery(eq("test query"), eq(15));
verify(client, times(1)).basicQuery(eq("test query"), eq(15), eq(false));
verify(client, times(1)).nextPage(any());
verifyNoMoreInteractions(client);
}
Expand All @@ -107,7 +107,7 @@ public void testCursorCleanupOnError() throws Exception {
HttpClient client = mock(HttpClient.class);
CliSession cliSession = new CliSession(client);
cliSession.setFetchSize(15);
when(client.basicQuery("test query", 15)).thenReturn(fakeResponse("my_cursor1", true, "first"));
when(client.basicQuery("test query", 15, false)).thenReturn(fakeResponse("my_cursor1", true, "first"));
when(client.nextPage("my_cursor1")).thenThrow(new SQLException("test exception"));
when(client.queryClose("my_cursor1", Mode.CLI)).thenReturn(true);
ServerQueryCliCommand cliCommand = new ServerQueryCliCommand();
Expand All @@ -118,7 +118,7 @@ public void testCursorCleanupOnError() throws Exception {
first \s
<b>Bad request [</b><i>test exception</i><b>]</b>
""", testTerminal.toString());
verify(client, times(1)).basicQuery(eq("test query"), eq(15));
verify(client, times(1)).basicQuery(eq("test query"), eq(15), eq(false));
verify(client, times(1)).nextPage(any());
verify(client, times(1)).queryClose(eq("my_cursor1"), eq(Mode.CLI));
verifyNoMoreInteractions(client);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ public MainResponse serverInfo() throws SQLException {
}

public SqlQueryResponse basicQuery(String query, int fetchSize) throws SQLException {
return basicQuery(query, fetchSize, CoreProtocol.FIELD_MULTI_VALUE_LENIENCY);
}

public SqlQueryResponse basicQuery(String query, int fetchSize, boolean fieldMultiValueLeniency) throws SQLException {
// TODO allow customizing the time zone - this is what session set/reset/get should be about
// method called only from CLI
SqlQueryRequest sqlRequest = new SqlQueryRequest(
Expand All @@ -74,7 +78,7 @@ public SqlQueryResponse basicQuery(String query, int fetchSize) throws SQLExcept
Boolean.FALSE,
null,
new RequestInfo(Mode.CLI, ClientVersion.CURRENT),
false,
fieldMultiValueLeniency,
false,
cfg.binaryCommunication()
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ private void assertBinaryRequestForCLI(XContentType xContentType) throws URISynt

prepareMockResponse();
try {
httpClient.basicQuery(query, fetchSize);
httpClient.basicQuery(query, fetchSize, false);
} catch (SQLException e) {
logger.info("Ignored SQLException", e);
}
Expand Down