QL: Add leniency option to SQL CLI #83795

luigidellaquila · 2022-02-10T16:28:46Z

Add lenient = true/false command to CLI client to allow execution of lenient queries on multi-value fields.

Fixes #67436

and fix test cases

elasticmachine · 2022-02-10T16:28:48Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2022-02-10T16:29:31Z

Hi @luigidellaquila, I've created a changelog YAML for you.

astefan

Looked at half of the code and, before proceeding further, I am wondering if lenient shouldn't have been a command (to be used inside the CLI) and not a CLI option (to be used as a parameter when the CLI is started).

...ck/plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/cli/SelectTestCase.java

x-pack/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java

astefan · 2022-02-10T17:46:23Z

x-pack/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java

@@ -102,6 +103,11 @@ public Cli(CliTerminal cliTerminal) {
            .withRequiredArg()
            .ofType(Boolean.class)
            .defaultsTo(Boolean.parseBoolean(System.getProperty("cli.check", "true")));
+        this.lenientOption = parser.acceptsAll(


Looking at the already existent commands and options for CLI, I would have inclined to make lenient a command and not a cli option. For example, fetch_size is a command (FetchSizeCliCommand class). The command can be executed multiple times during the life of the cli and an user can change the leniency with each query executed.

From the session configuration point of view (and maybe consistency?), this would make indeed sense.
From a practical POV, not sure if changing this behavior on a query-by-query case would be as useful, but for those users that use the CLI integrated into other scripts, having it as an argument might be preferred.

I agree, having a command would make it much more flexible, I'm implementing it now.
Do you think it makes sense to keep both (ie. the command AND the cli option) or should we just have the command?

It doesn't make sense to have it in both places. We are not doing this for any setting/config so far. Please, implement it as a command only.

OK, I removed the CLI option and replaced it with a command for now (lenient = true/false).
In case we decide to go with both, re-enabling the option is straight forward.

astefan

Looking better. Left few more comments.

...ck/plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/cli/SelectTestCase.java

x-pack/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java

astefan · 2022-02-11T10:28:14Z

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

    private boolean debug;
    private boolean binary;
+    private boolean lenient = CoreProtocol.FIELD_MULTI_VALUE_LENIENCY;


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.

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

.../sql/sql-client/src/test/java/org/elasticsearch/xpack/sql/client/HttpClientRequestTests.java

luigidellaquila · 2022-02-11T12:17:08Z

@elasticmachine update branch

extract a class to hold the configuration

luigidellaquila · 2022-02-11T15:19:50Z

@elasticmachine update branch

Luegg

LGTM

.../sql/sql-client/src/test/java/org/elasticsearch/xpack/sql/client/HttpClientRequestTests.java

astefan

Thank you for incorporating the reviews. I've left two more comments.

.../sql/sql-client/src/test/java/org/elasticsearch/xpack/sql/client/HttpClientRequestTests.java

astefan · 2022-02-14T13:16:44Z

...ck/plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/cli/SelectTestCase.java

@@ -37,4 +37,14 @@ public void testSelectWithWhere() throws IOException {
        assertThat(readLine(), RegexMatcher.matches("\\s*2\\s*\\|\\s*test_value2\\s*"));
        assertEquals("", readLine());
    }
+


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).

costin

LGTM - the only comment I would make is to rename CliSession#getConfiguration to cfg or configuration() since the config object is internal and final and cannot be set.
I'm late to this PR and it seems to be CliSession == CliConfiguration at the stage (CliSession doesn't really add anything extra and it has the same scope as configuration); however in the future there might be other things to add to it so I'm fine with the refactoring.

astefan

LGTM

…ijun/elasticsearch into fix-none-tsdb-index-dimension-tests * 'fix-none-tsdb-index-dimension-tests' of github.com:weizijun/elasticsearch: (37 commits) [docs] Mention JDK 17 in the Contributing docs (elastic#84018) Fix GeoIpDownloader startup during rolling upgrade (elastic#84000) Script: Fields API for Dense Vector (elastic#83550) Move InferenceConfigUpdate under VersionedNamedWriteable (elastic#84022) [ML] Fix license feature test cleanup (elastic#84020) Replace deprecated api in artifact transforms (elastic#84015) QL: Add leniency option to SQL CLI (elastic#83795) [Stack Monitoring] add kibana_stats version alias to -mb template (elastic#83930) Optimize spliterator for ImmutableOpenMap (elastic#83899) Feature usage actions for archive (elastic#83931) Use latch to speedup multi feature migration test (elastic#84007) Make action names available in NodeClient (elastic#83919) [DOCS] Re-add HTTP proxy setings from elastic#82737 (elastic#84001) Add CI matrix configuration for snapshot BWC versions (elastic#83990) Update YAML Rest tests to check for product header on all responses (elastic#83290) TSDB: Add time series aggs cancellation (elastic#83492) [DOCS] Fix percolate query headings (elastic#83988) [DOCS] Move tip for percolate query example (elastic#83972) Simplify LocalExporter cleaner function to fix failing tests (elastic#83812) [GCE Discovery] Correcly handle large zones with 500 or more instances (elastic#83785) ...

luigidellaquila added 3 commits February 10, 2022 13:57

Add leniency option to SQL CLI

58de769

Set default leniency to false for SQL CLI

36740fe

and fix test cases

Add integration tests for CLI leniency

4905bbe

luigidellaquila added >enhancement :Analytics/SQL SQL querying Team:QL (Deprecated) Meta label for query languages team labels Feb 10, 2022

luigidellaquila requested review from costin, astefan and bpintea February 10, 2022 16:28

elasticsearchmachine added the v8.2.0 label Feb 10, 2022

Update docs/changelog/83795.yaml

fbdb378

astefan requested a review from Luegg February 10, 2022 16:57

astefan reviewed Feb 10, 2022

View reviewed changes

Implement leniency option as a CLI command

68e197c

luigidellaquila requested a review from astefan February 11, 2022 09:47

luigidellaquila added 2 commits February 11, 2022 11:17

Remove unused code

db58495

Fix LenientCliCommand javadoc

2e8b6f0

astefan reviewed Feb 11, 2022

View reviewed changes

luigidellaquila added 2 commits February 11, 2022 11:46

Add unit test for LenientCliCommand

88a55b8

Code cleanup and improve test checks

876fbb9

elasticmachine and others added 2 commits February 11, 2022 06:17

Merge branch 'master' into leniency_on_sql_cli

0a1aacd

Refactor CLI session configuration

01fdbb4

extract a class to hold the configuration

Merge branch 'master' into leniency_on_sql_cli

55ee671

luigidellaquila requested a review from astefan February 11, 2022 16:43

Luegg approved these changes Feb 11, 2022

View reviewed changes

.../sql/sql-client/src/test/java/org/elasticsearch/xpack/sql/client/HttpClientRequestTests.java Outdated Show resolved Hide resolved

astefan reviewed Feb 14, 2022

View reviewed changes

costin approved these changes Feb 14, 2022

View reviewed changes

Create specific class for CLI Lenient IT tests

42dde1e

luigidellaquila requested a review from astefan February 15, 2022 10:14

astefan approved these changes Feb 16, 2022

View reviewed changes

luigidellaquila merged commit e6abd9f into elastic:master Feb 16, 2022

QL: Add leniency option to SQL CLI #83795

QL: Add leniency option to SQL CLI #83795

Uh oh!

Conversation

luigidellaquila commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 10, 2022

Uh oh!

elasticsearchmachine commented Feb 10, 2022

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpintea Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luigidellaquila commented Feb 11, 2022

Uh oh!

luigidellaquila commented Feb 11, 2022

Uh oh!

Luegg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luigidellaquila commented Feb 10, 2022 •

edited

Loading

bpintea Feb 10, 2022 •

edited

Loading