Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,6 @@ public void testSelectColumnFromMissingIndex() throws SQLException {
}
}

public void testSelectFromEmptyIndex() throws IOException, SQLException {
// Create an index without any types
Request request = new Request("PUT", "/test");
request.setJsonEntity("{}");
client().performRequest(request);

try (Connection c = esJdbc()) {
SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT * FROM test").executeQuery());
assertEquals("Found 1 problem\nline 1:8: Cannot determine columns for [*]", e.getMessage());
}
}

public void testSelectColumnFromEmptyIndex() throws IOException, SQLException {
Request request = new Request("PUT", "/test");
request.setJsonEntity("{}");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ public interface ErrorsTestCase {

void testSelectColumnFromMissingIndex() throws Exception;

void testSelectFromEmptyIndex() throws Exception;

void testSelectColumnFromEmptyIndex() throws Exception;

void testSelectMissingField() throws Exception;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,6 @@ public void testSelectColumnFromMissingIndex() throws Exception {
assertEquals("line 1:17: Unknown index [test]" + END, readLine());
}

@Override
public void testSelectFromEmptyIndex() throws Exception {
// Create an index without any types
Request request = new Request("PUT", "/test");
request.setJsonEntity("{}");
client().performRequest(request);

assertFoundOneProblem(command("SELECT * FROM test"));
assertEquals("line 1:8: Cannot determine columns for [*]" + END, readLine());
}

@Override
public void testSelectColumnFromEmptyIndex() throws Exception {
Request request = new Request("PUT", "/test");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,15 @@ protected static void loadEmpDatasetIntoEs(RestClient client) throws Exception {
// frozen index
loadEmpDatasetIntoEs(client, "frozen_emp", "employees");
freeze(client, "frozen_emp");
loadNoColsDatasetIntoEs(client, "no_cols");
}

private static void loadNoColsDatasetIntoEs(RestClient client, String index) throws Exception {
createEmptyIndex(client, index);
Request request = new Request("POST", "/" + index + "/_doc");
XContentBuilder emptyDoc = JsonXContent.contentBuilder().startObject().endObject();
request.setJsonEntity(Strings.toString(emptyDoc));
client.performRequest(request);
}

public static void loadDocsDatasetIntoEs(RestClient client) throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ public abstract class SqlSpecTestCase extends SpecBaseIntegrationTestCase {
private String query;

@ClassRule
public static LocalH2 H2 = new LocalH2((c) -> { c.createStatement().execute("RUNSCRIPT FROM 'classpath:/setup_test_emp.sql'"); });
public static LocalH2 H2 = new LocalH2((c) -> {
c.createStatement().execute("RUNSCRIPT FROM 'classpath:/setup_test_emp.sql'");
c.createStatement().execute("RUNSCRIPT FROM 'classpath:/setup_no_cols.sql'");
});

@ParametersFactory(argumentFormatting = PARAM_FORMATTING)
public static List<Object[]> readScriptSpec() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -355,16 +355,6 @@ public void testSelectColumnFromMissingIndex() throws Exception {
expectBadRequest(() -> runSql(mode, "SELECT abc FROM missing"), containsString("1:17: Unknown index [missing]"));
}

@Override
public void testSelectFromEmptyIndex() throws Exception {
// Create an index without any types
Request request = new Request("PUT", "/test");
request.setJsonEntity("{}");
client().performRequest(request);
String mode = randomFrom("jdbc", "plain");
expectBadRequest(() -> runSql(mode, "SELECT * FROM test"), containsString("1:8: Cannot determine columns for [*]"));
}

@Override
public void testSelectColumnFromEmptyIndex() throws Exception {
Request request = new Request("PUT", "/test");
Expand Down
25 changes: 21 additions & 4 deletions x-pack/plugin/sql/qa/server/src/main/resources/command.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -223,13 +223,14 @@ TODAY |SCALAR
showTables
SHOW TABLES;

name | type | kind
name | type | kind
logs |TABLE |INDEX
logs_nanos |TABLE |INDEX
no_cols |TABLE |INDEX
test_alias |VIEW |ALIAS
test_alias_emp |VIEW |ALIAS
test_emp |TABLE |INDEX
test_emp_copy |TABLE |INDEX
test_alias_emp |VIEW |ALIAS
test_emp |TABLE |INDEX
test_emp_copy |TABLE |INDEX
;

showTablesSimpleLike
Expand Down Expand Up @@ -382,3 +383,19 @@ last_name |VARCHAR |text
last_name.keyword |VARCHAR |keyword
salary |INTEGER |integer
;


describeNoCols
DESCRIBE "no_cols";

column:s | type:s | mapping:s
----------------------+---------------+---------------
;


showColumnsInNoCols
SHOW COLUMNS IN "no_cols";

column:s | type:s | mapping:s
----------------------+---------------+---------------
;
23 changes: 23 additions & 0 deletions x-pack/plugin/sql/qa/server/src/main/resources/no-columns.csv-spec
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
selectConstAggregationWithGroupBy
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

SELECT 1 a, COUNT(*) b, MAX(1) c FROM no_cols GROUP BY a;

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

subselectWithConst
SELECT 1, * FROM (SELECT * FROM no_cols) s;

1:i
---------------
1
;

subselectWithInnerConst
SELECT * FROM (SELECT 1, * FROM no_cols) s;

1:i
---------------
1
;
21 changes: 21 additions & 0 deletions x-pack/plugin/sql/qa/server/src/main/resources/no-columns.sql-spec
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
selectStar
SELECT * FROM no_cols;
selectStarWithFilter
SELECT * FROM no_cols WHERE 2 > 1;
selectStarWithFilterAndLimit
SELECT * FROM no_cols WHERE 1 > 2 LIMIT 10;

selectCount
SELECT COUNT(*) FROM no_cols;
// awaits fix: https://github.com/elastic/elasticsearch/issues/74311
// selectCountWithWhere
// SELECT COUNT(*) FROM no_cols WHERE 1 + 1 = 3;

selectConst
SELECT 1, 2, 3 FROM no_cols;
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Luegg Luegg Jun 21, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thx!

// subselect
// SELECT * FROM (SELECT * FROM no_cols) s;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
DROP TABLE IF EXISTS "no_cols";
CREATE TABLE "no_cols" ();

INSERT INTO "no_cols" DEFAULT VALUES;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
DROP TABLE IF EXISTS "test_emp";
CREATE TABLE "test_emp" (
"birth_date" TIMESTAMP WITH TIME ZONE,
"emp_no" INT,
"emp_no" INT,
"first_name" VARCHAR(50),
"gender" VARCHAR(1),
"hire_date" TIMESTAMP WITH TIME ZONE,
Expand All @@ -10,4 +10,5 @@ CREATE TABLE "test_emp" (
"name" VARCHAR(50),
"salary" INT
)
AS SELECT birth_date, emp_no, first_name, gender, hire_date, languages, last_name, CONCAT(first_name, ' ', last_name) AS name, salary FROM CSVREAD('classpath:/employees.csv');
AS SELECT birth_date, emp_no, first_name, gender, hire_date, languages, last_name, CONCAT(first_name, ' ', last_name) AS name, salary FROM CSVREAD('classpath:/employees.csv');

Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@
showTables
SHOW TABLES INCLUDE FROZEN;

name | type | kind
name | type | kind
frozen_emp |TABLE |FROZEN INDEX
logs |TABLE |INDEX
logs_nanos |TABLE |INDEX
no_cols |TABLE |INDEX
test_alias |VIEW |ALIAS
test_alias_emp |VIEW |ALIAS
test_emp |TABLE |INDEX
test_emp_copy |TABLE |INDEX
test_alias_emp |VIEW |ALIAS
test_emp |TABLE |INDEX
test_emp_copy |TABLE |INDEX
;

columnFromFrozen
Expand All @@ -31,18 +32,18 @@ F
percentileFrozen
SELECT gender, PERCENTILE(emp_no, 92.45) p1 FROM FROZEN frozen_emp GROUP BY gender;

gender:s | p1:d
null |10018.745
gender:s | p1:d
null |10018.745
F |10098.0085
M |10091.393
M |10091.393
;

countFromFrozen
SELECT gender, COUNT(*) AS c FROM FROZEN frozen_emp GROUP BY gender;

gender:s | c:l
null |10
F |33
null |10
F |33
M |57
;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -417,8 +417,9 @@ private List<NamedExpression> expandProjections(List<? extends NamedExpression>
for (NamedExpression ne : projections) {
if (ne instanceof UnresolvedStar) {
List<NamedExpression> expanded = expandStar((UnresolvedStar) ne, output);

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

Choose a reason for hiding this comment

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

why not keep the empty list vs a null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, makes sense 👍

result.add(ne);
} else {
result.addAll(expanded);
Expand All @@ -442,7 +443,7 @@ private List<NamedExpression> expandStar(UnresolvedStar us, List<Attribute> outp
// a qualifier is specified - since this is a star, it should be a CompoundDataType
if (us.qualifier() != null) {
// resolve the so-called qualifier first
// since this is an unresolved start we don't know whether it's a path or an actual qualifier
// since this is an unresolved star we don't know whether it's a path or an actual qualifier
Attribute q = resolveAgainstList(us.qualifier(), output, true);

// the wildcard couldn't be expanded because the field doesn't exist at all
Expand All @@ -455,6 +456,10 @@ private List<NamedExpression> expandStar(UnresolvedStar us, List<Attribute> outp
else if (q.resolved() == false) {
return singletonList(q);
}
// qualifier resolves to a non-struct field and cannot be expanded
else if (DataTypes.isPrimitive(q.dataType())) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}

// now use the resolved 'qualifier' to match
for (Attribute attr : output) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,9 @@ public List<Tuple<Integer, Comparator>> sortingColumns() {
*/
public BitSet columnMask(List<Attribute> columns) {
BitSet mask = new BitSet(fields.size());
aliasName(columns.get(0));
if(columns.size() > 0) {
aliasName(columns.get(0));
}

for (Attribute column : columns) {
Expression expression = aliases.resolve(column, column);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.elasticsearch.xpack.sql.parser.SqlParser;
import org.elasticsearch.xpack.sql.stats.Metrics;

import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -301,4 +302,15 @@ public void testFunctionWithExpressionOverNonExistingFieldAsArgumentAndSameAlias
assertEquals("Found 1 problem\nline 1:22: Unknown column [missing]", ex.getMessage());
}

public void testExpandStarOnIndexWithoutColumns() {
EsIndex test = new EsIndex("test", Collections.emptyMap());
getIndexResult = IndexResolution.valid(test);
analyzer = new Analyzer(SqlTestUtils.TEST_CFG, functionRegistry, getIndexResult, verifier);

LogicalPlan plan = plan("SELECT * FROM test");

assertThat(plan, instanceOf(Project.class));
assertTrue(((Project) plan).projections().isEmpty());
}

}