Details
-
Bug
-
Status: Closed
-
Major
-
Resolution: Fixed
-
1.10.0
-
None
-
None
Description
Drill is unable to resolve temporary table if default temporary workspace is partially indicated (schema was set using USE command and only workspace name is used in query).
0: jdbc:drill:zk=local> use dfs; +-------+----------------------------------+ | ok | summary | +-------+----------------------------------+ | true | Default schema changed to [dfs] | +-------+----------------------------------+ 1 row selected (0.156 seconds) 0: jdbc:drill:zk=local> create temporary table tmp.t as select 'A' from (values(1)); +-----------+----------------------------+ | Fragment | Number of records written | +-----------+----------------------------+ | 0_0 | 1 | +-----------+----------------------------+ 1 row selected (1.525 seconds) 0: jdbc:drill:zk=local> select * from tmp.t; Feb 02, 2017 11:24:09 AM org.apache.calcite.sql.validate.SqlValidatorException <init> SEVERE: org.apache.calcite.sql.validate.SqlValidatorException: Table 'tmp.t' not found Feb 02, 2017 11:24:09 AM org.apache.calcite.runtime.CalciteException <init> SEVERE: org.apache.calcite.runtime.CalciteContextException: From line 1, column 15 to line 1, column 17: Table 'tmp.t' not found Error: VALIDATION ERROR: From line 1, column 15 to line 1, column 17: Table 'tmp.t' not found SQL Query null [Error Id: 5266cb67-9d37-4a94-9a4e-28a4a2f94be5 on localhost:31010] (state=,code=0)
Attachments
Issue Links
- links to
Activity
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/736#discussion_r99162943
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java —
@@ -527,5 +529,31 @@ public RelOptTableImpl getTable(final List<String> names) {
}
return super.getTable(names);
}
+
+ /**
+ * Check if passed table is temporary or not should be done if:
+ * <li>schema is not indicated (only one element in the names list)<li/>
+ * <li>current schema or indicated schema is default temporary workspace<li/>
+ *
+ * Examples (where dfs.tmp is default temporary workspace):
+ * <li>select * from t<li/>
+ * <li>select * from dfs.tmp.t<li/>
+ * <li>use dfs; select * from tmp.t<li/>
+ *
+ * @param names list of schema and table names, table name is always the last element
+ * @param defaultSchemaPath current schema path set using USE command
+ * @param drillConfig drill config
+ * @return true if check for temporary table should be done, false otherwise
+ */
+ private boolean checkForTemporaryTable(List<String> names, String defaultSchemaPath, DrillConfig drillConfig) {
— End diff –
static
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/736#discussion_r99163906
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java —
@@ -527,5 +529,31 @@ public RelOptTableImpl getTable(final List<String> names) {
}
return super.getTable(names);
}
+
+ /**
+ * Check if passed table is temporary or not should be done if:
+ * <li>schema is not indicated (only one element in the names list)<li/>
+ * <li>current schema or indicated schema is default temporary workspace<li/>
+ *
+ * Examples (where dfs.tmp is default temporary workspace):
+ * <li>select * from t<li/>
+ * <li>select * from dfs.tmp.t<li/>
+ * <li>use dfs; select * from tmp.t<li/>
+ *
+ * @param names list of schema and table names, table name is always the last element
+ * @param defaultSchemaPath current schema path set using USE command
+ * @param drillConfig drill config
+ * @return true if check for temporary table should be done, false otherwise
+ */
+ private boolean checkForTemporaryTable(List<String> names, String defaultSchemaPath, DrillConfig drillConfig) {
— End diff –
`checkForTemporaryTable` is just private method used only in `DrillCalciteCatalogReader` class. I don't think it should be `private static.`
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/736#discussion_r99164906
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java —
@@ -527,5 +529,31 @@ public RelOptTableImpl getTable(final List<String> names) {
}
return super.getTable(names);
}
+
+ /**
+ * Check if passed table is temporary or not should be done if:
+ * <li>schema is not indicated (only one element in the names list)<li/>
+ * <li>current schema or indicated schema is default temporary workspace<li/>
+ *
+ * Examples (where dfs.tmp is default temporary workspace):
+ * <li>select * from t<li/>
+ * <li>select * from dfs.tmp.t<li/>
+ * <li>use dfs; select * from tmp.t<li/>
+ *
+ * @param names list of schema and table names, table name is always the last element
+ * @param defaultSchemaPath current schema path set using USE command
+ * @param drillConfig drill config
+ * @return true if check for temporary table should be done, false otherwise
+ */
+ private boolean checkForTemporaryTable(List<String> names, String defaultSchemaPath, DrillConfig drillConfig) {
— End diff –
To emphasize that this method does not rely on any internal state; it's fine either way.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/736#discussion_r99166435
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java —
@@ -527,5 +529,31 @@ public RelOptTableImpl getTable(final List<String> names) {
}
return super.getTable(names);
}
+
+ /**
+ * Check if passed table is temporary or not should be done if:
+ * <li>schema is not indicated (only one element in the names list)<li/>
+ * <li>current schema or indicated schema is default temporary workspace<li/>
+ *
+ * Examples (where dfs.tmp is default temporary workspace):
+ * <li>select * from t<li/>
+ * <li>select * from dfs.tmp.t<li/>
+ * <li>use dfs; select * from tmp.t<li/>
+ *
+ * @param names list of schema and table names, table name is always the last element
+ * @param defaultSchemaPath current schema path set using USE command
+ * @param drillConfig drill config
+ * @return true if check for temporary table should be done, false otherwise
+ */
+ private boolean checkForTemporaryTable(List<String> names, String defaultSchemaPath, DrillConfig drillConfig) {
— End diff –
Makes sense. I'll make a change.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/736#discussion_r99167324
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java —
@@ -527,5 +529,31 @@ public RelOptTableImpl getTable(final List<String> names) {
}
return super.getTable(names);
}
+
+ /**
+ * Check if passed table is temporary or not should be done if:
+ * <li>schema is not indicated (only one element in the names list)<li/>
+ * <li>current schema or indicated schema is default temporary workspace<li/>
+ *
+ * Examples (where dfs.tmp is default temporary workspace):
+ * <li>select * from t<li/>
+ * <li>select * from dfs.tmp.t<li/>
+ * <li>use dfs; select * from tmp.t<li/>
+ *
+ * @param names list of schema and table names, table name is always the last element
+ * @param defaultSchemaPath current schema path set using USE command
+ * @param drillConfig drill config
+ * @return true if check for temporary table should be done, false otherwise
+ */
+ private boolean checkForTemporaryTable(List<String> names, String defaultSchemaPath, DrillConfig drillConfig) {
— End diff –
Oh, actually I can't since `DrillCalciteCatalogReader` is inner class and inner classes can't have static declarations
Changed Reviewer to sudheeshkatkam, since he's provided review comments.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/736#discussion_r99388535
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java —
@@ -527,5 +529,31 @@ public RelOptTableImpl getTable(final List<String> names) {
}
return super.getTable(names);
}
+
+ /**
+ * Check if passed table is temporary or not should be done if:
+ * <li>schema is not indicated (only one element in the names list)<li/>
+ * <li>current schema or indicated schema is default temporary workspace<li/>
+ *
+ * Examples (where dfs.tmp is default temporary workspace):
+ * <li>select * from t<li/>
+ * <li>select * from dfs.tmp.t<li/>
+ * <li>use dfs; select * from tmp.t<li/>
+ *
+ * @param names list of schema and table names, table name is always the last element
+ * @param defaultSchemaPath current schema path set using USE command
+ * @param drillConfig drill config
+ * @return true if check for temporary table should be done, false otherwise
+ */
+ private boolean checkForTemporaryTable(List<String> names, String defaultSchemaPath, DrillConfig drillConfig) {
+ if (names.size() == 1)
— End diff –
A single name might be a temporary table, or it might not be. Hence the name suggestion above. Is the intent to simply apply a filter so that we separate out those that are not temporary tables from those that may or may not be?
If we want a positive check that this is, in fact, a temp table, wouldn't we have to resolve the name against known temp tables rather than just infer based on path length?
And, if we are just filtering out known non-temp tables, is that good enough for the use case? That is, can the case we are fixing handle the ambiguous "may or may not be temporary" case?
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/736#discussion_r99387926
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java —
@@ -527,5 +529,31 @@ public RelOptTableImpl getTable(final List<String> names) {
}
return super.getTable(names);
}
+
+ /**
+ * Check if passed table is temporary or not should be done if:
+ * <li>schema is not indicated (only one element in the names list)<li/>
+ * <li>current schema or indicated schema is default temporary workspace<li/>
+ *
+ * Examples (where dfs.tmp is default temporary workspace):
+ * <li>select * from t<li/>
+ * <li>select * from dfs.tmp.t<li/>
+ * <li>use dfs; select * from tmp.t<li/>
+ *
+ * @param names list of schema and table names, table name is always the last element
+ * @param defaultSchemaPath current schema path set using USE command
+ * @param drillConfig drill config
+ * @return true if check for temporary table should be done, false otherwise
+ */
+ private boolean checkForTemporaryTable(List<String> names, String defaultSchemaPath, DrillConfig drillConfig) {
— End diff –
`checkForTemporaryTable` --> `isTemporaryTable` ?
Actually, given the first condition, is this `mightBeTemporaryTable`?
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/736#discussion_r99393786
— Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java —
@@ -527,5 +529,31 @@ public RelOptTableImpl getTable(final List<String> names) {
}
return super.getTable(names);
}
+
+ /**
+ * Check if passed table is temporary or not should be done if:
+ * <li>schema is not indicated (only one element in the names list)<li/>
+ * <li>current schema or indicated schema is default temporary workspace<li/>
+ *
+ * Examples (where dfs.tmp is default temporary workspace):
+ * <li>select * from t<li/>
+ * <li>select * from dfs.tmp.t<li/>
+ * <li>use dfs; select * from tmp.t<li/>
+ *
+ * @param names list of schema and table names, table name is always the last element
+ * @param defaultSchemaPath current schema path set using USE command
+ * @param drillConfig drill config
+ * @return true if check for temporary table should be done, false otherwise
+ */
+ private boolean checkForTemporaryTable(List<String> names, String defaultSchemaPath, DrillConfig drillConfig) {
+ if (names.size() == 1)
— End diff –
Renamed method to `mightBeTemporaryTable`. You are right the intent here is just to filter out cases when we don't need to check is passed table might be temporary. When table is passed to `getTableName` we don't know if it's temporary or not. If it is temporary we need to substitute the name to generated one and there are several indicators when we need to check this: table name is without schema or indicated schema is default temporary workspace,in all other cases we just check regular tables directly.
Merged into master with commit id 1ec3edf01575f302b765b62317ca16c1547dbe10
Verified on Drill 1.10.0 commit ID : 300e9349
apache drill 1.10.0-SNAPSHOT "a drill is a terrible thing to waste" 0: jdbc:drill:schema=dfs.tmp> use dfs; +-------+----------------------------------+ | ok | summary | +-------+----------------------------------+ | true | Default schema changed to [dfs] | +-------+----------------------------------+ 1 row selected (0.367 seconds) 0: jdbc:drill:schema=dfs.tmp> create temporary table tmp.t as select 'A' from (values(1)); +-----------+----------------------------+ | Fragment | Number of records written | +-----------+----------------------------+ | 0_0 | 1 | +-----------+----------------------------+ 1 row selected (0.486 seconds) 0: jdbc:drill:schema=dfs.tmp> select * from tmp.t; +---------+ | EXPR$0 | +---------+ | A | +---------+ 1 row selected (0.21 seconds) 0: jdbc:drill:schema=dfs.tmp>
GitHub user arina-ielchiieva opened a pull request:
https://github.com/apache/drill/pull/736
DRILL-5238: CTTAS: unable to resolve temporary table if workspace is ……indicated without schema
1. Added additional check for passed table is temporary if default workspace is partially indicated (issue is described in Jira
DRILL-5238).2. Removed `UserSession.getDefaultSchemaName()` method as it contained exactly the same logic as `UserSession.getDefaultSchemaPath()`.
3. Since `UserSession.properties` are set using builder, they are not required but when not set, they may cause NPE when `UserSession.getProp()` method is called. Moved `UserSession.properties` into constructor.
4. Unit test for partial schema usage with temporary table.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arina-ielchiieva/drill
DRILL-5238Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/736.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #736
commit a787c6c2b49de6b8114f028864ff8d67cc5989be
Author: Arina Ielchiieva <arina.yelchiyeva@gmail.com>
Date: 2017-02-02T11:47:19Z
DRILL-5238: CTTAS: unable to resolve temporary table if workspace is indicated without schema