Skip to content

Druid Avatica - Handle escaping of search characters correctly #10040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
samarthjain merged 3 commits into from
Jun 18, 2020
Merged

Druid Avatica - Handle escaping of search characters correctly #10040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8b8dc3
Fix Avatica based metadata queries by appending ESCAPE '\' clause to …
samarthjain Jun 17, 2020
72d8f2a
Add ESCAPE \ clause to getSchemas() and getTables()
samarthjain Jun 17, 2020
6b67acc
Adjust test to incorporate new datasources
samarthjain Jun 17, 2020
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
18 changes: 12 additions & 6 deletions sql/src/main/java/org/apache/druid/sql/avatica/DruidMeta.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ public MetaResultSet getSchemas(
}

if (schemaPattern.s != null) {
whereBuilder.add("SCHEMATA.SCHEMA_NAME LIKE " + Calcites.escapeStringLiteral(schemaPattern.s));
whereBuilder.add("SCHEMATA.SCHEMA_NAME LIKE " + withEscapeClause(schemaPattern.s));
}

final String where = whereBuilder.isEmpty() ? "" : "WHERE " + Joiner.on(" AND ").join(whereBuilder);
Expand Down Expand Up @@ -395,11 +395,11 @@ public MetaResultSet getTables(
}

if (schemaPattern.s != null) {
whereBuilder.add("TABLES.TABLE_SCHEMA LIKE " + Calcites.escapeStringLiteral(schemaPattern.s));
whereBuilder.add("TABLES.TABLE_SCHEMA LIKE " + withEscapeClause(schemaPattern.s));
}

if (tableNamePattern.s != null) {
whereBuilder.add("TABLES.TABLE_NAME LIKE " + Calcites.escapeStringLiteral(tableNamePattern.s));
whereBuilder.add("TABLES.TABLE_NAME LIKE " + withEscapeClause(tableNamePattern.s));
}

if (typeList != null) {
Expand Down Expand Up @@ -446,15 +446,16 @@ public MetaResultSet getColumns(
}

if (schemaPattern.s != null) {
whereBuilder.add("COLUMNS.TABLE_SCHEMA LIKE " + Calcites.escapeStringLiteral(schemaPattern.s));
Copy link
Copy Markdown
Contributor

@gianm gianm Jun 17, 2020

Choose a reason for hiding this comment

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

This looks like the right fix. Could you please make it to all of getSchemas, getTables, and getColumns? They have the same problem. It might be nice to create a private static helper function in this file to avoid all the boilerplate, too.

Copy link
Copy Markdown
Contributor Author

@samarthjain samarthjain Jun 17, 2020

Choose a reason for hiding this comment

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

Thanks for taking a look, @gianm . Pushed a commit to add the ESCAPE clause in the three methods along with tests.

whereBuilder.add("COLUMNS.TABLE_SCHEMA LIKE " + withEscapeClause(schemaPattern.s));
}

if (tableNamePattern.s != null) {
whereBuilder.add("COLUMNS.TABLE_NAME LIKE " + Calcites.escapeStringLiteral(tableNamePattern.s));
whereBuilder.add("COLUMNS.TABLE_NAME LIKE " + withEscapeClause(tableNamePattern.s));
}

if (columnNamePattern.s != null) {
whereBuilder.add("COLUMNS.COLUMN_NAME LIKE " + Calcites.escapeStringLiteral(columnNamePattern.s));
whereBuilder.add("COLUMNS.COLUMN_NAME LIKE "
+ withEscapeClause(columnNamePattern.s));
}

final String where = whereBuilder.isEmpty() ? "" : "WHERE " + Joiner.on(" AND ").join(whereBuilder);
Expand Down Expand Up @@ -638,4 +639,9 @@ private int getEffectiveMaxRowsPerFrame(int clientMaxRowsPerFrame)
}
return Math.min(clientMaxRowsPerFrame, config.getMaxRowsPerFrame());
}

private static String withEscapeClause(String toEscape)
{
return Calcites.escapeStringLiteral(toEscape) + " ESCAPE '\\'";
}
}
230 changes: 228 additions & 2 deletions sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,8 +414,19 @@ public void testDatabaseMetaDataTables() throws Exception
Pair.of("TABLE_NAME", CalciteTests.DATASOURCE3),
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_TYPE", "TABLE")
),
row(
Pair.of("TABLE_CAT", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_TYPE", "TABLE")
),
row(
Pair.of("TABLE_CAT", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_TYPE", "TABLE")
)

),
getRows(
metaData.getTables(null, "druid", "%", null),
Expand Down Expand Up @@ -465,8 +476,19 @@ public void testDatabaseMetaDataTablesAsSuperuser() throws Exception
Pair.of("TABLE_NAME", CalciteTests.DATASOURCE3),
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_TYPE", "TABLE")
),
row(
Pair.of("TABLE_CAT", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_TYPE", "TABLE")
),
row(
Pair.of("TABLE_CAT", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_TYPE", "TABLE")
)

),
getRows(
metaData.getTables(null, "druid", "%", null),
Expand Down Expand Up @@ -957,6 +979,210 @@ public void testSysTableParameterBinding() throws Exception
);
}

@Test
public void testEscapingForGetColumns() throws Exception
{
final DatabaseMetaData metaData = client.getMetaData();

ImmutableList<Map<String, Object>> someDatasourceColumns = ImmutableList.of(
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "__time")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "cnt")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "dim1")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "dim2")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "dim3")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "m1")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "m2")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "unique_dim1")
)
);
// If the escape clause wasn't correctly set, rows for potentially none or more than
// one datasource (some_datasource and somexdatasource) would have been returned
Assert.assertEquals(
someDatasourceColumns,
getRows(
metaData.getColumns(null, "dr_id", CalciteTests.SOME_DATSOURCE_ESCAPED, null),
ImmutableSet.of("TABLE_NAME", "TABLE_SCHEM", "COLUMN_NAME")
)
);

ImmutableList<Map<String, Object>> someXDatasourceColumns = ImmutableList.of(
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("COLUMN_NAME", "__time")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("COLUMN_NAME", "cnt_x")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("COLUMN_NAME", "m1_x")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("COLUMN_NAME", "m2_x")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("COLUMN_NAME", "unique_dim1_x")
)
);
Assert.assertEquals(
someXDatasourceColumns,
getRows(
metaData.getColumns(null, "dr_id", "somexdatasource", null),
ImmutableSet.of("TABLE_NAME", "TABLE_SCHEM", "COLUMN_NAME")
)
);

List<Map<String, Object>> columnsOfBothTables = new ArrayList<>(someDatasourceColumns);
columnsOfBothTables.addAll(someXDatasourceColumns);
// Assert that the pattern matching still works when no escape string is provided
Assert.assertEquals(
columnsOfBothTables,
getRows(
metaData.getColumns(null, "dr_id", "some_datasource", null),
ImmutableSet.of("TABLE_NAME", "TABLE_SCHEM", "COLUMN_NAME")
)
);

// Assert column name pattern works correctly when _ is in the column names
Assert.assertEquals(
ImmutableList.of(
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("COLUMN_NAME", "m1_x")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("COLUMN_NAME", "m2_x")
)
),
getRows(
metaData.getColumns("druid", "dr_id", CalciteTests.SOMEXDATASOURCE, "m_\\_x"),
ImmutableSet.of("TABLE_NAME", "TABLE_SCHEM", "COLUMN_NAME")
)
);

// Assert column name pattern with % works correctly for column names starting with m
Assert.assertEquals(
ImmutableList.of(
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "m1")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE),
Pair.of("COLUMN_NAME", "m2")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("COLUMN_NAME", "m1_x")
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE),
Pair.of("COLUMN_NAME", "m2_x")
)
),
getRows(
metaData.getColumns("druid", "dr_id", CalciteTests.SOME_DATASOURCE, "m%"),
ImmutableSet.of("TABLE_NAME", "TABLE_SCHEM", "COLUMN_NAME")
)
);
}

@Test
public void testEscapingForGetTables() throws Exception
{
final DatabaseMetaData metaData = client.getMetaData();

Assert.assertEquals(
ImmutableList.of(
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE)
)
),
getRows(
metaData.getTables("druid", "dr_id", CalciteTests.SOME_DATSOURCE_ESCAPED, null),
ImmutableSet.of("TABLE_SCHEM", "TABLE_NAME")
)
);

Assert.assertEquals(
ImmutableList.of(
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE)
)
),
getRows(
metaData.getTables("druid", "dr_id", CalciteTests.SOMEXDATASOURCE, null),
ImmutableSet.of("TABLE_SCHEM", "TABLE_NAME")
)
);

// Assert that some_datasource is treated as a pattern that matches some_datasource and somexdatasource
Assert.assertEquals(
ImmutableList.of(
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOME_DATASOURCE)
),
row(
Pair.of("TABLE_SCHEM", "druid"),
Pair.of("TABLE_NAME", CalciteTests.SOMEXDATASOURCE)
)
),
getRows(
metaData.getTables("druid", "dr_id", CalciteTests.SOME_DATASOURCE, null),
ImmutableSet.of("TABLE_SCHEM", "TABLE_NAME")
)
);
}

private static List<Map<String, Object>> getRows(final ResultSet resultSet) throws SQLException
{
return getRows(resultSet, null);
Expand Down
4 changes: 4 additions & 0 deletions sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,8 @@ public void testInformationSchemaTables() throws Exception
.add(new Object[]{"druid", CalciteTests.DATASOURCE4, "TABLE"})
.add(new Object[]{"druid", CalciteTests.DATASOURCE5, "TABLE"})
.add(new Object[]{"druid", CalciteTests.DATASOURCE3, "TABLE"})
.add(new Object[]{"druid", CalciteTests.SOME_DATASOURCE, "TABLE"})
.add(new Object[]{"druid", CalciteTests.SOMEXDATASOURCE, "TABLE"})
.add(new Object[]{"druid", "aview", "VIEW"})
.add(new Object[]{"druid", "bview", "VIEW"})
.add(new Object[]{"INFORMATION_SCHEMA", "COLUMNS", "SYSTEM_TABLE"})
Expand Down Expand Up @@ -746,6 +748,8 @@ public void testInformationSchemaTables() throws Exception
.add(new Object[]{"druid", CalciteTests.FORBIDDEN_DATASOURCE, "TABLE"})
.add(new Object[]{"druid", CalciteTests.DATASOURCE5, "TABLE"})
.add(new Object[]{"druid", CalciteTests.DATASOURCE3, "TABLE"})
.add(new Object[]{"druid", CalciteTests.SOME_DATASOURCE, "TABLE"})
.add(new Object[]{"druid", CalciteTests.SOMEXDATASOURCE, "TABLE"})
.add(new Object[]{"druid", "aview", "VIEW"})
.add(new Object[]{"druid", "bview", "VIEW"})
.add(new Object[]{"INFORMATION_SCHEMA", "COLUMNS", "SYSTEM_TABLE"})
Expand Down
Loading