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
