DOC: Add examples to the SQL docs #31633

costin · 2018-06-27T21:08:01Z

Wip on the documentation examples.
Put the tests into a separate suite to help with clarity and isolation.
The existing tests have different CVS tests (that include data type among other things) which are not relevant for the docs.
Also moved the library dataset into the test suite.
Additionally, used the same output from CLI in the JDBC test suite (a different PR should eliminate the JdbcTestUtils output entirely and only use the CliFormatter).

Once the infrastructure is in place, adding the examples goes quite smooth. I also like the SQL with table approach since it's easier to read (and applicable for all consumers - REST, JDBC, CLI).

elasticmachine · 2018-06-27T21:08:02Z

Pinging @elastic/es-search-aggs

nik9000

I'm fine with new CSV files and a new data set. Could we get away without DocsCsvSpecTestCase/JdbcDocCsvSpectIT if we do it during the regular CSV tests. If we're worried about time we could use `-Dtests.method='docs'?

nik9000 · 2018-06-27T21:45:09Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DataLoader.java

        makeAlias(client, "test_alias", "test_emp", "test_emp_copy");
        makeAlias(client, "test_alias_emp", "test_emp", "test_emp_copy");
    }

+    protected static void loadDocsDatasetIntoEs(RestClient client) throws Exception {
+        loadEmpDatasetIntoEs(client, "emp");


I'd prefer to rename the one in tests to emp and load the library one for all the tests.

costin · 2018-06-27T22:10:50Z

I ended up with a different test since for tests we have a number of indices that show up in SHOW TABLES or any type of FROM emp* query.
We have test_emp, test_emp_copy plus an alias to them.
If library and emp are placed in the same suite, the tests for SHOW TABLES need to be changed both in the docs and in the tests.
So in the interest of time, having this namespace between the test suite (that might get geo in the future, maybe the flight data) and the docs seems reasonable and not too much effort.
It can be definitely be revisited in the future but as of right now, I could come up with a single test suite and isolation between the datasets so they don't leak into each others commands.

nik9000

Fine by me. I'd prefer to figure out a way around it, but that can come later. I left one important thing and one minor structural thing.

nik9000 · 2018-06-28T16:50:09Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DocsCsvSpecTestCase.java

+        // uncomment this to printout the result set and create new CSV tests
+        //
+        JdbcTestUtils.logLikeCLI(elastic, log);
+        //JdbcAssert.assertResultSets(expected, elastic, log);


This look like it isn't asserting anything. I don't mind the printing being enabled all the time but I think we should have the assert on, right?

You're right - the comment will disappear in the final commit. I'm printing stuff out to help with the docs (logging while asserting adds the log header which makes it harder to copy-paste the text output).

nik9000 · 2018-06-28T16:50:24Z

x-pack/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/DocsCsvSpecTestCase.java

+ * That's not to say the two cannot be merged however that felt like too much of an effort
+ * at this stage and, to not keep things stalling, started with this approach.
+ */
+public abstract class DocsCsvSpecTestCase extends SpecBaseIntegrationTestCase {


Could you flatten this into into the JDBC spec test case so we don't have an abstract class with a single non-abstract child?

Roger that.

Significantly improve the example snippets in the documentation. The examples are part of the test suite and checked nightly. To help readability, the existing dataset was extended (test_emp renamed to emp plus library).

Add examples to all sections Fix two minor bugs in the JDBC driver discovered Improve output of JDBC tests to be consistent with the CLI Add lenient flag to JDBC asserts to allow type widening (a long is equivalent to a integer as long as the value is the same)

costin · 2018-07-02T19:40:41Z

Pushed the changes (including a rebase from master which looks like it didn't delete the existing comments probably because the commit itself was kept in place).

I've improved the tests and added example through-out the rest of the docs.
I've done a couple of improvements to the Jdbc tests mainly aligning its output with that of the CLI so one gets the same output regardless.
This lead to discovering two bugs in the driver (see #31734) so I picked up their changes (as they are small) since otherwise checking the CSV spec failed.

nik9000

I left a few questions. I still think the approach is good but have some questions about smaller parts of it.

nik9000 · 2018-07-02T20:34:31Z

docs/reference/sql/language/syntax/select.asciidoc

-// TESTRESPONSE[_cat]
+["source","sql",subs="attributes,callouts,macros"]
+----
+include-tagged::{sql-specs}/docs.csv-spec[orderByScore]


nik9000 · 2018-07-02T20:35:02Z

x-pack/qa/sql/build.gradle

@@ -10,6 +10,8 @@ dependencies {

  // JDBC testing dependencies
  compile project(path: xpackModule('sql:jdbc'), configuration: 'nodeps')
+  compile project(path: xpackModule('sql:sql-proto'))


What makes this required?

It's not needed anymore since we depend on sql-action (it's redundant) so I removed it.

nik9000 · 2018-07-02T20:35:09Z

x-pack/qa/sql/build.gradle

@@ -85,6 +88,8 @@ subprojects {
    }
    testCompile "org.elasticsearch.test:framework:${version}"

+    testCompile project(path: xpackModule('sql:sql-proto'))


What about this one?

Removed as well (since everything got folded back to qa-sql).

nik9000 · 2018-07-02T20:36:31Z

...l/no-security/src/test/java/org/elasticsearch/xpack/qa/sql/nosecurity/JdbcDocCsvSpectIT.java

+        // uncomment this to printout the result set and create new CSV tests
+        //
+        //JdbcTestUtils.logLikeCLI(elastic, log);
+        JdbcAssert.assertResultSets(expected, elastic, log, true);


So the headers don't have to encode the type?

If the lenient is set to true, the types are widened and only the values are compared.

nik9000 · 2018-07-02T20:37:24Z

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

@@ -344,7 +344,7 @@ public Object getObject(int columnIndex) throws SQLException {
            throw new SQLException("type is null");
        }

-        return getObject(columnIndex, type);
+        return convert(columnIndex, type);


I think this includes the jdbc fix, right?

Yes, that's the one (that triggers the StackOverflow).

astefan · 2018-07-02T21:55:08Z

docs/reference/sql/language/syntax/select.asciidoc

@@ -83,17 +86,30 @@ where:
 `table_name`::

 Represents the name (optionally qualified) of an existing table, either a concrete or base one (actual index) or alias.
+
+
 If the table name contains special SQL characters (such as `.`,`-`,etc...) use double quotes to escape them:


Maybe worth specifying a more complete list of special characters that need escaping?

That needs to be a separate section (regarding the grammar in general) for a future PR.

astefan · 2018-07-02T22:16:43Z

docs/reference/sql/language/syntax/select.asciidoc

+["source","sql",subs="attributes,callouts,macros"]
+----
+include-tagged::{sql-specs}/docs.csv-spec[orderByScoreWithMatch]
+----

 NOTE:
 Trying to return `score` from a non full-text queries will return the same value for all results, as


Small typo I believe for the query-queries: "from a non full-text queries". Should probably be singular "query" or, if using plural, "from non full-text queries".

Also, "equilley" -> "equally".

Significantly improve the example snippets in the documentation. The examples are part of the test suite and checked nightly. To help readability, the existing dataset was extended (test_emp renamed to emp plus library). Improve output of JDBC tests to be consistent with the CLI Add lenient flag to JDBC asserts to allow type widening (a long is equivalent to a integer as long as the value is the same). (cherry picked from commit de9e56a)

Significantly improve the example snippets in the documentation. The examples are part of the test suite and checked nightly. To help readability, the existing dataset was extended (test_emp renamed to emp plus library). Improve output of JDBC tests to be consistent with the CLI Add lenient flag to JDBC asserts to allow type widening (a long is equivalent to a integer as long as the value is the same). (cherry picked from commit de9e56a) (cherry picked from commit 77f5e17) (cherry picked from commit 093ea03) (cherry picked from commit b342552)

* 6.x: Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) (#31789) [Docs] Correct default window_size (#31582) S3 fixture should report 404 on unknown bucket (#31782) [ML] Limit ML filter items to 10K (#31731) Fixture for Minio testing (#31688) [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647) [DOCS] Add missing get mappings docs to HLRC (#31765) [DOCS] Starting Elasticsearch (#31701) Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747) Painless: Complete Removal of Painless Type (#31699) Consolidate watcher setting update registration (#31762) [DOCS] Adds empty 6.3.1 release notes page ingest: Introduction of a bytes processor (#31733) [test] don't run bats tests for suse boxes (#31749) Add analyze API to high-level rest client (#31577) Implemented XContent serialisation for GetIndexResponse (#31675) [DOCS] Typos DOC: Add examples to the SQL docs (#31633) Add support for AWS session tokens (#30414) Watcher: Reenable start/stop yaml tests (#31754) JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735) Support multiple system store types (#31650) Add write*Blob option to replace existing blob (#31729) Split CircuitBreaker-related tests (#31659) Painless: Add Context Docs (#31190) Docs: Remove missing reference Migrate scripted metric aggregation scripts to ScriptContext design (#30111) Watcher: Fix chain input toXcontent serialization (#31721) Remove _all example (#31711) rest-high-level: added get cluster settings (#31706) Docs: Match the examples in the description (#31710) [Docs] Correct typos (#31720) Extend allowed characters for grok field names (#21745) (#31653) (#31722) [DOCS] Check for Windows and *nix file paths (#31648) [ML] Validate ML filter_id (#31535) Fix gradle4.8 deprecation warnings (#31654) Update numbers to reflect 4-byte UTF-8-encoded characters (#27083)

* master: [ML] Rate limit established model memory updates (#31768) [Docs] Correct default window_size (#31582) S3 fixture should report 404 on unknown bucket (#31782) Detach Transport from TransportService (#31727) [ML] Limit ML filter items to 10K (#31731) [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647) Fixture for Minio testing (#31688) [DOCS] Add missing get mappings docs to HLRC (#31765) [DOCS] Starting Elasticsearch (#31701) Painless: Complete Removal of Painless Type (#31699) Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) Consolidate watcher setting update registration (#31762) Build: re-enabled bwc (#31769) ingest: Introduction of a bytes processor (#31733) Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747) Add analyze API to high-level rest client (#31577) [DOCS] Typos DOC: Add examples to the SQL docs (#31633) Add support for AWS session tokens (#30414) Watcher: Reenable start/stop yaml tests (#31754) Implemented XContent serialisation for GetIndexResponse (#31675) JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735) resolveHasher defaults to NOOP (#31723) Account for XContent overhead in in-flight breaker Split CircuitBreaker-related tests (#31659) Add write*Blob option to replace existing blob (#31729) Painless: Add Context Docs (#31190) Watcher: Fix chain input toXcontent serialization (#31721) Docs: Match the examples in the description (#31710) rest-high-level: added get cluster settings (#31706) [Docs] Correct typos (#31720) Clean up double semicolon code typos (#31687) [DOCS] Check for Windows and *nix file paths (#31648) [ML] Validate ML filter_id (#31535) Revert long lines Fix TransportChangePasswordActionTests

costin added >docs General docs changes v7.0.0 :Analytics/SQL SQL querying v6.4.0 labels Jun 27, 2018

costin self-assigned this Jun 27, 2018

costin requested review from nik9000 and astefan June 27, 2018 21:08

nik9000 reviewed Jun 27, 2018

View reviewed changes

nik9000 requested changes Jun 28, 2018

View reviewed changes

costin added 2 commits July 2, 2018 19:49

DOC: Add examples to the SQL docs

877f09a

Significantly improve the example snippets in the documentation. The examples are part of the test suite and checked nightly. To help readability, the existing dataset was extended (test_emp renamed to emp plus library).

Complete examples

7872e25

Add examples to all sections Fix two minor bugs in the JDBC driver discovered Improve output of JDBC tests to be consistent with the CLI Add lenient flag to JDBC asserts to allow type widening (a long is equivalent to a integer as long as the value is the same)

costin force-pushed the doc-examples branch from 18574af to 7872e25 Compare July 2, 2018 19:36

costin added 3 commits July 2, 2018 22:50

Add javadocs

b740e32

Merge remote-tracking branch 'remotes/upstream/master' into doc-examples

389592b

Add back setup in build for the REST docs

1a5c54a

nik9000 reviewed Jul 2, 2018

View reviewed changes

Removed unneeded dependencies

184f448

astefan reviewed Jul 2, 2018

View reviewed changes

nik9000 approved these changes Jul 3, 2018

View reviewed changes

costin merged commit de9e56a into elastic:master Jul 3, 2018

costin deleted the doc-examples branch July 3, 2018 14:24

costin added the v6.3.1 label Jul 3, 2018

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Add examples to the SQL docs #31633

DOC: Add examples to the SQL docs #31633

costin commented Jun 27, 2018

elasticmachine commented Jun 27, 2018

nik9000 left a comment

nik9000 Jun 27, 2018

costin commented Jun 27, 2018

nik9000 left a comment

nik9000 Jun 28, 2018

costin Jun 29, 2018

nik9000 Jun 28, 2018

costin Jun 29, 2018

costin commented Jul 2, 2018

nik9000 left a comment

nik9000 Jul 2, 2018

nik9000 Jul 2, 2018

costin Jul 2, 2018

nik9000 Jul 2, 2018

costin Jul 2, 2018

nik9000 Jul 2, 2018

costin Jul 2, 2018

nik9000 Jul 2, 2018

costin Jul 2, 2018

astefan Jul 2, 2018

costin Jul 3, 2018

astefan Jul 2, 2018

astefan Jul 2, 2018

DOC: Add examples to the SQL docs #31633

DOC: Add examples to the SQL docs #31633

Conversation

costin commented Jun 27, 2018

elasticmachine commented Jun 27, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin commented Jun 27, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin commented Jul 2, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment