jnturton commented on code in PR #2733: URL: https://github.com/apache/drill/pull/2733#discussion_r1068252216
########## exec/java-exec/src/main/java/org/apache/drill/exec/work/metadata/MetadataProvider.java: ########## @@ -607,6 +608,16 @@ public String getQueryUserName() { @Override public UserCredentials getQueryUserCredentials() { return session.getCredentials(); } + + @Override + public String getTemporaryTableName(String table) { Review Comment: This looks like another case where we wouldn't need to keep expanding interfaces like SchemaConfigInfoProvider and adding partial implementations where some throw UnsupportedOperationExceptions if we just had a good way of accessing the UserSession from most layers of Drill. It's not something for this PR for sure, but I wanted to remark to get your opinion since I remember having to work the same way when I was trying to expose UserCredentials (visible above) for user translation in plugins. ########## exec/java-exec/src/main/java/org/apache/drill/exec/work/metadata/MetadataProvider.java: ########## @@ -607,6 +608,16 @@ public String getQueryUserName() { @Override public UserCredentials getQueryUserCredentials() { return session.getCredentials(); } + + @Override + public String getTemporaryTableName(String table) { + return session.resolveTemporaryTableName(table); + } + + @Override + public String getTemporaryWorkspace() { + return config.getString(ExecConstants.DEFAULT_TEMPORARY_WORKSPACE); Review Comment: Have I got it right that this config option value is the only value returned by implementations of getTemporaryWorkspace? If so, do we need this method or could its callers look up the config value themselves instead? ########## exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/conversion/DrillCalciteCatalogReader.java: ########## @@ -135,14 +112,15 @@ public Prepare.PreparingTable getTable(List<String> names) { } private void checkTemporaryTable(List<String> names) { - if (allowTemporaryTables) { + if (allowTemporaryTables || !needsTemporaryTableCheck(names, session.getDefaultSchemaPath(), drillConfig)) { return; } - String originalTableName = session.getOriginalTableNameFromTemporaryTable(names.get(names.size() - 1)); + String tableName = names.get(names.size() - 1); + String originalTableName = session.resolveTemporaryTableName(tableName); if (originalTableName != null) { throw UserException .validationError() - .message("Temporary tables usage is disallowed. Used temporary table name: [%s].", originalTableName) + .message("Temporary tables usage is disallowed. Used temporary table name: [%s].", tableName) Review Comment: ```suggestion .message("A reference to temporary table [%s] was made in a context where temporary table references are not allowed.", tableName) ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org