msqe-case-insensitive-enabled #14830
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14830 +/- ##
============================================
+ Coverage 61.75% 63.72% +1.97%
- Complexity 207 1612 +1405
============================================
Files 2436 2708 +272
Lines 133233 151297 +18064
Branches 20636 23359 +2723
============================================
+ Hits 82274 96416 +14142
- Misses 44911 47632 +2721
- Partials 6048 7249 +1201
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Jackie-Jiang
added
backward-incompat
| .collect(Collectors.toSet()); | ||
| //return _tableCache.getTableNameMap().keySet().stream().filter(n -> DatabaseUtils.isPartOfDatabase(n, _databaseName)) | ||
| // .collect(Collectors.toSet()); | ||
| return _tableCache.getTableNameMap().keySet(); |
There was a problem hiding this comment.
Enabling calcite case-insensitive configuration led to a lot of unexpected consequences. One of them is that, during table name resolution and validation, it now relies in schema.getTableNames() to find if a table exists:
As it was implemented, this method got rid of all non-prefixed tableNames when _databaseName != null, and probably that was not intended.
There was a problem hiding this comment.
As it was implemented, this method got rid of all non-prefixed tableNames when _databaseName != null, and probably that was not intended.
The non-prefixed table names are assumed to belong to the "default" database IIUC. So shouldn't we want to exclude them when there's a non-default database being used?
cc - @shounakmk219
There was a problem hiding this comment.
Thanks for the patch @albertobastos! I've left a few minor comments and questions.
| @Test | ||
| public void testCaseInsensitiveNames() throws Exception { | ||
| String query = "select ACTualELAPsedTIMe from mYtABLE where actUALelAPSedTIMe > 0 limit 1"; | ||
| JsonNode jsonNode = postQuery(query); |
There was a problem hiding this comment.
This uses the broker query API; let's also add a call to the controller query API (via postQueryToController) to test that path as well here - I see that you've made the necessary changes for that to work already.
| @@ -118,9 +111,15 @@ public QueryEnvironment(Config config) { | |||
| String database = config.getDatabase(); | |||
| PinotCatalog catalog = new PinotCatalog(config.getTableCache(), database); | |||
| CalciteSchema rootSchema = CalciteSchema.createRootSchema(false, false, database, catalog); | |||
| Properties connectionConfigProperties = new Properties(); | |||
| connectionConfigProperties.setProperty(CalciteConnectionProperty.CASE_SENSITIVE.camelName(), Boolean.toString( | |||
| config.getTableCache() == null | |||
There was a problem hiding this comment.
Will the table cache ever be null here? Doesn't look like it should be.
| .collect(Collectors.toSet()); | ||
| //return _tableCache.getTableNameMap().keySet().stream().filter(n -> DatabaseUtils.isPartOfDatabase(n, _databaseName)) | ||
| // .collect(Collectors.toSet()); | ||
| return _tableCache.getTableNameMap().keySet(); |
There was a problem hiding this comment.
As it was implemented, this method got rid of all non-prefixed tableNames when _databaseName != null, and probably that was not intended.
The non-prefixed table names are assumed to belong to the "default" database IIUC. So shouldn't we want to exclude them when there's a non-default database being used?
cc - @shounakmk219
| "SELECT count(*) FROM mytable WHERE DaysSinceEpoch = " + daysSinceEpoch + " limit 10000", | ||
| "SELECT count(*) FROM mytable WHERE timeConvert(DaysSinceEpoch,'DAYS','HOURS') = " + hoursSinceEpoch | ||
| + " limit 10000", | ||
| "SELECT count(*) FROM mytable WHERE timeConvert(DaysSinceEpoch,'DAYS','SECONDS') = " + secondsSinceEpoch | ||
| + " limit 10000", | ||
| "SELECT MAX(timeConvert(DaysSinceEpoch,'DAYS','SECONDS')) FROM mytable limit 10000", |
There was a problem hiding this comment.
What are these changes for?
| testQueryError(query, QueryException.QUERY_PLANNING_ERROR_CODE); | ||
| //testQueryError(query, QueryException.QUERY_PLANNING_ERROR_CODE); | ||
| JsonNode response = postQuery(query); | ||
| assertTrue(response.get("numSegmentsProcessed").asLong() >= 1L, "Query: " + query + " failed"); |
There was a problem hiding this comment.
If the intention is to change the assertion to verify that the query no longer fails (since we're case insensitive by default now), we can use the assertNoError method here.
| List<String> queries = new ArrayList<>(); | ||
| baseQueries.forEach(q -> queries.add(q.replace("mytable", "MYTABLE").replace("DaysSinceEpoch", "DAYSSinceEpOch"))); | ||
| baseQueries.forEach( | ||
| q -> queries.add(q.replace("mytable", "MYDB.MYTABLE").replace("DaysSinceEpoch", "DAYSSinceEpOch"))); | ||
| q -> queries.add(q.replace("mytable", "DEFAULT.MYTABLE").replace("DaysSinceEpoch", "DAYSSinceEpOch"))); |
There was a problem hiding this comment.
Why did the database name need to be changed here?
closes #14675
Backwards compatibility:
When
enable.case.insensitive=true(the default value) responses using MSQE will return the contents oftablesQueriedalways in lowercase. That's due to how internally Calcite is used for column validation and how it handles a case-insensitive scenario. Given that it happens when a cluster is configured as case-insensitive, hopefully the consumer will also be prepared to ignore case when using that response field.