SQL: Fix catalog filtering in SYS COLUMNS by costin · Pull Request #40583 · elastic/elasticsearch

costin · 2019-03-28T10:55:02Z

Properly treat '%' as a wildcard for catalog filtering instead of doing
a straight string match.

Properly treat '%' as a wildcard for catalog filtering instead of doing a straight string match. Fix elastic#40582

elasticmachine · 2019-03-28T10:55:04Z

Pinging @elastic/es-search

Add integration test for SYS COLUMNS - currently running only inside single_node since the cluster name is test dependent.

costin · 2019-03-28T12:47:11Z

I have updated the PR to fix also alias filtering which was not performed (only the index name was considered). Also added a test suite since mocking doesn't really work for this case.

matriv · 2019-03-28T14:56:40Z

...in/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/SpecBaseIntegrationTestCase.java

    public static InputStream readFromJarUrl(URL source) throws IOException {
-        return source.openStream();
+        URLConnection con = source.openConnection();
+        con.setDefaultUseCaches(false);


Could you add a comment why this is necessary?

matriv · 2019-03-28T14:58:19Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/analysis/index/IndexResolver.java

+                boolean matchesAlias = false;
+                if (pattern != null && aliasMetadata != null) {
+                    for (AliasMetaData aliasMeta : aliasMetadata) {
+                        matchesAlias |= pattern.matcher(aliasMeta.alias()).matches();


Shouldn't we exit on first occurrence of true?

matriv · 2019-03-28T15:00:35Z

...l/qa/single-node/src/test/java/org/elasticsearch/xpack/sql/qa/single_node/JdbcCsvSpecIT.java

+    public static List<Object[]> readScriptSpec() throws Exception {
+        List<Object[]> list = new ArrayList<>();
+        list.addAll(CsvSpecTestCase.readScriptSpec());
+        return readScriptSpec("/disabled/command-sys.csv-spec", specParser());


I don't like disabled in the path here.

astefan

LGTM

matriv

LGTM

for the drivers) Move away from separateMappings() since in case of aliases pointing to multiple indices, it can return duplicated entries (as there's no merging) causing issues. Hence why mergedMapping is used instead with the assumption that only one table will be used at a time. A more correct (yet quite expensive) alternative would be to return the mapping per index yet in case of aliases, drop it and use field caps instead. However as this is not CCS-friendly, using field_caps instead seems a good solution for now.

Fix monotony of ORDINAL_POSITION

matriv

LGTM

astefan

Added two comments, but am not 100% sure I am right about those.

astefan · 2019-04-01T12:49:58Z

x-pack/plugin/sql/qa/src/main/resources/single-node-only/command-sys.csv-spec

+
+   TABLE_CAT:s |  TABLE_SCHEM:s|  TABLE_NAME:s |    COLUMN_NAME:s   |   DATA_TYPE:i |   TYPE_NAME:s |  COLUMN_SIZE:i| BUFFER_LENGTH:i|DECIMAL_DIGITS:i|NUM_PREC_RADIX:i | NULLABLE:i|    REMARKS:s  |  COLUMN_DEF:s |SQL_DATA_TYPE:i|SQL_DATETIME_SUB:i|CHAR_OCTET_LENGTH:i|ORDINAL_POSITION:i|IS_NULLABLE:s|SCOPE_CATALOG:s|SCOPE_SCHEMA:s|SCOPE_TABLE:s|SOURCE_DATA_TYPE:sh|IS_AUTOINCREMENT:s|IS_GENERATEDCOLUMN:s
+---------------+---------------+---------------+--------------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+---------------+----------------+-----------------+----------------+---------------+---------------+---------------+---------------+----------------+----------------+------------------
+x-pack_plugin_sql_qa_single-node_integTestCluster  |null           |test\_emp      |birth_date        |93             |DATETIME       |24             |8              |null           |null           |1              |null           |null           |9              |3               |null             |1               |YES            |null           |null           |null           |null            |NO              |NO                


I may miss something, but is it ok for this to return `test\_emp' (including the escaped character)?

This is a bug - the test\_emp is escaped so the name should be test_emp.

astefan · 2019-04-01T13:03:57Z

x-pack/plugin/sql/qa/src/main/resources/single-node-only/command-sys.csv-spec

+sysColumnsWithTableLikeNoEscape
+SYS COLUMNS TABLE LIKE 'test_emp';
+
+// since there's no escaping test_emp means test*emp which matches also test_alias_emp


test_emp should match test[one character]emp when there is no escape character, no?

Unfortunately due to the current field caps features, we can't figure out the source index.

bpintea · 2019-04-03T14:38:40Z

Gave the branch a try and if nothing was mistaken during the process:

sys columns catalog '%' table like 'employees' (and sys columns catalog '%' table like 'employees' like '%') returns an empty result set;
sys columns catalog 'distribution\_run' table like '%' will return a full list of all columns in all tables is the catalog (which is right); but sys columns catalog 'distribution\_run' table like '' will return the same, except for the table name column which is empty (which seems wrong).

bpintea · 2019-04-09T14:10:26Z

Update after consulting with @costin:

sys columns catalog '%' table like 'employees' (and sys columns catalog '%' table like 'employees' like '%') returns an empty result set;

This (=empty result set) is correct, since the catalog argument cannot contain a string search pattern.

sys columns catalog 'distribution\_run' table like '' will return the same, except for the table name column which is empty (which seems wrong)

This is currently a known limitation; however, it should have no practical impact, since the applications ask for the columns of specific table.

Properly treat '%' as a wildcard for catalog filtering instead of doing a straight string match. Table filtering now considers aliases as well. Add escaping char for LIKE queries with user defined params Fix monotony of ORDINAL_POSITION Add integration test for SYS COLUMNS - currently running only inside single_node since the cluster name is test dependent. Add pattern unescaping for index names Fix #40582 (cherry picked from commit 8e61b77)

Properly treat '%' as a wildcard for catalog filtering instead of doing a straight string match. Table filtering now considers aliases as well. Add escaping char for LIKE queries with user defined params Fix monotony of ORDINAL_POSITION Add integration test for SYS COLUMNS - currently running only inside single_node since the cluster name is test dependent. Add pattern unescaping for index names Fix elastic#40582

SQL: Fix catalog filtering in SYS COLUMNS

006adc7

Properly treat '%' as a wildcard for catalog filtering instead of doing a straight string match. Fix elastic#40582

costin added >bug v7.0.0 :Analytics/SQL SQL querying v8.0.0 v7.2.0 v6.7.1 labels Mar 28, 2019

costin requested review from astefan and bpintea March 28, 2019 10:55

costin requested a review from matriv March 28, 2019 10:55

Table filtering now considers aliases as well.

e753de2

Add integration test for SYS COLUMNS - currently running only inside single_node since the cluster name is test dependent.

matriv reviewed Mar 28, 2019

View reviewed changes

astefan approved these changes Mar 28, 2019

View reviewed changes

Address feedback

64da851

matriv approved these changes Mar 28, 2019

View reviewed changes

bpintea approved these changes Mar 28, 2019

View reviewed changes

costin added 4 commits March 28, 2019 19:29

Add escaping char for LIKE queries with user defined params

193c208

update tests

fa63177

Keep using separate mapping for getColumn '%'

0865ec9

Fix monotony of ORDINAL_POSITION

costin mentioned this pull request Mar 29, 2019

SQL: Use ResultSets over exceptions in metadata #40641

Merged

Merge branch 'master' into fix-40582

ff6fcfc

colings86 added v6.7.2 and removed v6.7.1 labels Mar 30, 2019

costin requested review from astefan, bpintea and matriv March 30, 2019 10:07

matriv approved these changes Apr 1, 2019

View reviewed changes

astefan reviewed Apr 1, 2019

View reviewed changes

costin added 2 commits April 8, 2019 23:45

Merge remote-tracking branch 'remotes/upstream/master' into fix-40582

597fb20

Try pattern unescaping

6c830b9

bpintea approved these changes Apr 9, 2019

View reviewed changes

costin merged commit 8e61b77 into elastic:master Apr 9, 2019

costin deleted the fix-40582 branch April 9, 2019 15:57

$@polyfractal$ polyfractal added v7.0.1 and removed v7.0.0 labels Apr 9, 2019

imotov mentioned this pull request May 8, 2019

SQL: Add back the single node JDBC CSV Spec tests #41960

Merged

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

Conversation

costin commented Mar 28, 2019

Uh oh!

elasticmachine commented Mar 28, 2019

Uh oh!

costin commented Mar 28, 2019

Uh oh!

matriv Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

matriv Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

matriv Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

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

astefan Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

costin Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

astefan Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

costin Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

bpintea commented Apr 3, 2019

Uh oh!

bpintea commented Apr 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants