SQL: Fix querying of indices without columns #74312

Luegg · 2021-06-18T14:36:17Z

Adds support for querying indices without columns. Previously, such queries failed.

elasticmachine · 2021-06-18T14:36:19Z

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

costin

LGTM.
Left a comment on null vs emptyList. Small nit, the table name could be improve to no_columns, no_mapping or empty_index.

costin · 2021-06-18T16:15:59Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

+
                    // the field exists, but cannot be expanded (no sub-fields)
-                    if (expanded.isEmpty()) {
+                    if (expanded == null) {


why not keep the empty list vs a null?

That's more or less what caused the bug. Previously, expansion errors resulted in an empty list but that's ambiguous if the star is expanded on an index without columns. In this case, it returns an empty list meaning "star has been expanded to no fields".

To distinguish between expansion errors and empty expansions I now had to encode the error case with a null return value.

You could return singletonList(ne) instead of passing null and treat it separately.

got it, makes sense 👍

costin · 2021-06-18T16:17:16Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java

                }
+                // qualifier resolves to a non-struct field and cannot be expanded
+                else if (DataTypes.isPrimitive(q.dataType())) {
+                    return null;


Use emptyList() instead of null - allows iteration without blowing up and it's still fast as it avoid any allocation.

I think you could simply return singleton(q) and avoid any special/null handling.

Luegg · 2021-06-18T16:52:44Z

LGTM.
Left a comment on null vs emptyList. Small nit, the table name could be improve to no_columns, no_mapping or empty_index.

I first used empty_index everywhere but then realized that this can mean either "empty mapping" or "no documents". Looking at the terminology in the docs, "empty_mapping" would probably be most accurate?

matriv

LGTM. Left a few comments though. Thx!

matriv · 2021-06-18T16:59:12Z

x-pack/plugin/sql/qa/server/src/main/resources/no-columns.csv-spec

@@ -0,0 +1,23 @@
+selectConstAggregationWithGroupBy


Why did you choose to have those in .csv-spec?

they also behave differently in H2 and all return no rows. I'll add a comment to clarify this.

matriv · 2021-06-18T17:00:36Z

x-pack/plugin/sql/qa/server/src/main/resources/setup_no_cols.sql

+DROP TABLE IF EXISTS "no_cols";
+CREATE TABLE "no_cols" ();
+
+INSERT INTO "no_cols" DEFAULT VALUES;


Is this required?

I also want to cover cases with empty documents indexed in the empty mapping index.

But now we only test a table with a row with no values, but not a completely empty table?

Yes that's true. Do you think it would be worth to also have an empty_empty_mapping index for covering the no rows cases? To some extent, the "no rows" cases are already covered by tests that filter out all records.

I don't know if that's too much, @costin, what do you think?

matriv · 2021-06-18T17:02:05Z

x-pack/plugin/sql/qa/server/src/main/resources/no-columns.sql-spec

+selectConstAggregation
+SELECT MAX(1), SUM(2) FROM no_cols;
+
+// fails in H2 but cannot be tested in CSV spec because datasets without columns cannot be parsed


Just curious, why does it fail in H2?

Maybe you can open an issue to improve testing and have a way to assert no cols in csv in the feature and link it here.

H2 fails with a syntax error:

Syntax error in SQL statement "SELECT FROM PUBLIC.""no_cols""[*]"; expected "(, USE, AS, RIGHT, LEFT, FULL, INNER, JOIN, CROSS, NATURAL, ,, SELECT"; SQL statement: CREATE FORCE VIEW PUBLIC._0 AS SELECT FROM PUBLIC."no_cols" [42001-197]

Looks like it uses a view to execute the subselect and the creation fails because it selects an empty list of columns. The latest version of H2 supports the query though, so #39895 would resolve the issue as well.

.../plugin/sql/src/main/java/org/elasticsearch/xpack/sql/querydsl/container/QueryContainer.java

bpintea

Nice. Lgtm.

astefan

I've left few comments, all related to the same issue basically.

astefan · 2021-06-22T09:53:17Z

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

+        Request request = new Request("POST", "/" + index + "/_bulk");
+        request.addParameter("refresh", "true");
+        request.setJsonEntity("{\"index\":{}\n{}\n" + "{\"index\":{}\n{}\n");
+        client.performRequest(request);


Why is this needed? Creating an empty index is not enough?

I think it's helpful to also check whether the behavior is correct with documents in an index with an empty mapping. That's why I'm indexing two empty documents.

Ok. Thanks.

astefan · 2021-06-22T09:54:27Z

x-pack/plugin/sql/qa/server/src/main/resources/empty-mapping.csv-spec

+
+       a:i     |       b:l     |       c:i
+---------------+---------------+---------------
+1              |2              |1


Why is this COUNT 2?

It's two empty docs in the index.

Sorry, missed that.

I also initially split the request body in two lines to make it more explicit like this:

request.setJsonEntity( "{\"index\":{}\n{}\n" + "{\"index\":{}\n{}\n");

but spotless didn't like it and I had to squash it into one line...

astefan · 2021-06-22T09:55:24Z

x-pack/plugin/sql/qa/server/src/main/resources/empty-mapping.csv-spec

+       1:i
+---------------
+1
+1


Second 1 - where does it come from?

astefan

LGTM

Fixes elastic#53001 Adds support for querying indices without columns. Previously, such queries failed. (cherry picked from commit 6f93303)

Fixes #53001 Adds support for querying indices without columns. Previously, such queries failed. (cherry picked from commit 6f93303)

joegallo · 2021-06-30T20:48:13Z

$ git log --oneline upstream/master | grep "Fix querying of indices without columns"
6f93303c36f SQL: Fix querying of indices without columns (#74312)
$ git log --oneline upstream/7.x | grep "Fix querying of indices without columns"
2bb34e48605 SQL: Fix querying of indices without columns (#74312) (#74466)
$ git log --oneline upstream/7.14 | grep "Fix querying of indices without columns"
2bb34e48605 SQL: Fix querying of indices without columns (#74312) (#74466)
$ git log --oneline upstream/7.13 | grep "Fix querying of indices without columns"
ae96371d822 SQL: Fix querying of indices without columns (#74312) (#74465)

SQL: Fix querying of indices without columns

f78559b

Luegg added >bug :Analytics/SQL SQL querying v8.0.0 Team:QL (Deprecated) Meta label for query languages team v7.14.0 v7.13.3 labels Jun 18, 2021

Luegg requested review from astefan, bpintea, costin and matriv June 18, 2021 14:36

Luegg added 2 commits June 18, 2021 16:57

address spotless issues

0e85eae

revert DebugSqlSpec changes

1928df4

costin approved these changes Jun 18, 2021

View reviewed changes

matriv approved these changes Jun 18, 2021

View reviewed changes

Luegg added 5 commits June 21, 2021 09:35

address comments

86c43c9

s/no_cols/empty_mapping/

868edd7

add missing setup script

46f77b9

spotless apply

19c54f5

return unresolved qualifier instead of null

4d1f92c

bpintea approved these changes Jun 21, 2021

View reviewed changes

astefan reviewed Jun 22, 2021

View reviewed changes

astefan approved these changes Jun 23, 2021

View reviewed changes

Luegg merged commit 6f93303 into elastic:master Jun 23, 2021

Luegg deleted the fix/selectIndexWoCols branch June 23, 2021 08:17

Luegg added the backport pending label Jun 23, 2021

Luegg pushed a commit that referenced this pull request Jun 23, 2021

SQL: Fix querying of indices without columns (#74312) (#74466)

2bb34e4

Fixes #53001 Adds support for querying indices without columns. Previously, such queries failed. (cherry picked from commit 6f93303)

Luegg pushed a commit that referenced this pull request Jun 23, 2021

SQL: Fix querying of indices without columns (#74312) (#74465)

ae96371

Fixes #53001 Adds support for querying indices without columns. Previously, such queries failed. (cherry picked from commit 6f93303)

joegallo removed the backport pending label Jun 30, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

+:i
+              ---------------
+
+

SQL: Fix querying of indices without columns #74312

SQL: Fix querying of indices without columns #74312

Uh oh!

Conversation

Luegg commented Jun 18, 2021

Uh oh!

elasticmachine commented Jun 18, 2021

Uh oh!

costin left a comment

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!

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!

Luegg commented Jun 18, 2021

Uh oh!

matriv left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Luegg Jun 21, 2021 • 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!

Uh oh!

bpintea 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!

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!

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!

joegallo commented Jun 30, 2021

Uh oh!

Reviewers

Assignees

Luegg Jun 21, 2021 •

edited

Loading