[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[HanumathRao]

Github user HanumathRao closed the pull request at:

https://github.com/apache/drill/pull/996


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[HanumathRao]

Github user HanumathRao commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r147896927
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/store/dfs/TestFileSelection.java
 ---
@@ -63,4 +63,17 @@ public void testEmptyFolderThrowsTableNotFound() throws 
Exception {
 }
   }
 
--- End diff --

done


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[HanumathRao]

Github user HanumathRao commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r147818548
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SchemaUtilites.java
 ---
@@ -77,6 +77,22 @@ public static SchemaPlus findSchema(final SchemaPlus 
defaultSchema, final String
 return findSchema(defaultSchema, schemaPathAsList);
   }
 
+  /**
+   * Utility function to get the commonPrefix schema between two supplied 
schemas.
--- End diff --

done.


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r146816101
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/store/dfs/TestFileSelection.java
 ---
@@ -63,4 +63,17 @@ public void testEmptyFolderThrowsTableNotFound() throws 
Exception {
 }
   }
 
+  @Test(expected = Exception.class)
+  public void testWrongSchemaThrowsSchemaNotFound() throws Exception {
+final String table = String.format("%s/empty", 
TestTools.getTestResourcesPath());
+final String query = String.format("select * from dfs1.`%s`", table);
+try {
+  testNoResult(query);
+} catch (Exception ex) {
+  final String pattern = String.format("[[dfs1]] is not valid with 
respect to either root schema or current default schema").toLowerCase();
+  final boolean isSchemaNotFound = 
ex.getMessage().toLowerCase().contains(pattern);
+  assertTrue(isSchemaNotFound);
+  throw ex;
+}
+  }
--- End diff --

Can you please add test case for the incorrect workspace, 
a. `select * from dfs.incorrect_wk.table;`
b. 
```
use dfs; 
select * from incorrect_wk.table;
```
I assume it will return incorrect schema exception as well? 


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r146818260
  
--- Diff: 
exec/java-exec/src/test/java/org/apache/drill/exec/store/dfs/TestFileSelection.java
 ---
@@ -63,4 +63,17 @@ public void testEmptyFolderThrowsTableNotFound() throws 
Exception {
 }
   }
 
--- End diff --

Maybe it makes sense to move new unit tests and 
`testEmptyFolderThrowsTableNotFound` into separate class which will test 
behavior when incorrect object is defined (i.e. schema, workspace, table)?


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r146816584
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SchemaUtilites.java
 ---
@@ -77,6 +77,22 @@ public static SchemaPlus findSchema(final SchemaPlus 
defaultSchema, final String
 return findSchema(defaultSchema, schemaPathAsList);
   }
 
+  /**
+   * Utility function to get the commonPrefix schema between two supplied 
schemas.
--- End diff --

Could you please add example in Java doc?


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r146815141
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java
 ---
@@ -481,6 +485,19 @@ public RelOptTableImpl getTable(final List 
names) {
 .message("Temporary tables usage is disallowed. Used temporary 
table name: %s.", names)
 .build(logger);
   }
+
+  // Check the schema and throw a valid SchemaNotFound exception 
instead of TableNotFound exception.
--- End diff --

Could you please factor out this logic in a separate method?


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[HanumathRao]

Github user HanumathRao commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r146118431
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SchemaUtilites.java
 ---
@@ -77,6 +77,22 @@ public static SchemaPlus findSchema(final SchemaPlus 
defaultSchema, final String
 return findSchema(defaultSchema, schemaPathAsList);
   }
 
+  /**
+   * Utility function to get the commonPrefix schema between two supplied 
schemas.
+   * @param defaultSchema default schema
+   * @param schemaPath current schema path
+   * @param isCaseSensitive true if caseSensitive comparision is required.
+   * @return common prefix schemaPath
+   */
+  public static String getPrefixSchemaPath(final String defaultSchema, 
final String schemaPath, final boolean isCaseSensitive) {
+if (!isCaseSensitive) {
+  return Strings.commonPrefix(defaultSchema.toLowerCase(), 
schemaPath.toLowerCase());
--- End diff --

@paul-rogers  Thank you for this catch. I did try out some examples which 
were throwing schemanotfound exception. I have changed the code to handle it. 
getDefaultSchemaPath of session object returns the defaultSchema as String, 
hence I converted the SchemaPath to String by SchemaPathJOINER. As pointed out 
by your comment, this will loose some context information if the "." is part of 
the name. However, I also think that there is a bug in the defaultSchema code 
path as it is also loosing this information by converting it to String. For now 
I am not fixing that bug (which I am planning to fix in another JIRA) as I am 
not sure   about the changes in setting defaultSchema.

Coming on to the specific valid use case which was throwing schemaNotFound 
exception is
in dfs I have introduced "tmp.tmp1" in place of tmp.

And if I query select * from dfs.`tmp.tmp1`.`employee.json` limit 1; this 
query reports SchemaNotFound exception even though a workspace with name 
tmp.tmp1 exists.

This is currently working with new changes.


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[HanumathRao]

Github user HanumathRao commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r146118332
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SchemaUtilites.java
 ---
@@ -77,6 +77,22 @@ public static SchemaPlus findSchema(final SchemaPlus 
defaultSchema, final String
 return findSchema(defaultSchema, schemaPathAsList);
   }
 
+  /**
+   * Utility function to get the commonPrefix schema between two supplied 
schemas.
+   * @param defaultSchema default schema
+   * @param schemaPath current schema path
+   * @param isCaseSensitive true if caseSensitive comparision is required.
+   * @return common prefix schemaPath
+   */
+  public static String getPrefixSchemaPath(final String defaultSchema, 
final String schemaPath, final boolean isCaseSensitive) {
--- End diff --

@paul-rogers  Thanks for the review. For this API I am assuming the caller 
will supply the information whether it needs to be a case sensitive or case 
insensitive comparison. The caller for this function is passing the 
parserConfig case sensitive parameter.


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r145565621
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SchemaUtilites.java
 ---
@@ -149,6 +168,16 @@ public static void throwSchemaNotFoundException(final 
SchemaPlus defaultSchema,
 .build(logger);
   }
 
+  /** Utility method to throw {@link UserException} with context 
information */
+  public static void throwSchemaNotFoundException(final String 
defaultSchema, final String givenSchemaPath) {
+throw UserException.validationError()
+.message("Schema [%s] is not valid with respect to either root 
schema or current default schema.",
--- End diff --

Because names can contain dots, we must more careful format the schema. The 
schema should consists of a list (array) of name parts. For each name:

* If the name contains a dot, wrap it in back ticks: `` `a.b` ``
* Otherwise, use the name as is: `c`
* Concatenate the name parts with dots: `` `a.b`.c ``

Code to do this might already exist. @vvysotskyi may know.


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r145564952
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SchemaUtilites.java
 ---
@@ -77,6 +77,22 @@ public static SchemaPlus findSchema(final SchemaPlus 
defaultSchema, final String
 return findSchema(defaultSchema, schemaPathAsList);
   }
 
+  /**
+   * Utility function to get the commonPrefix schema between two supplied 
schemas.
+   * @param defaultSchema default schema
+   * @param schemaPath current schema path
+   * @param isCaseSensitive true if caseSensitive comparision is required.
+   * @return common prefix schemaPath
+   */
+  public static String getPrefixSchemaPath(final String defaultSchema, 
final String schemaPath, final boolean isCaseSensitive) {
+if (!isCaseSensitive) {
+  return Strings.commonPrefix(defaultSchema.toLowerCase(), 
schemaPath.toLowerCase());
--- End diff --

Is the schema a single name? Probably not since we are talking about common 
prefixes.

Drill supports names with dots. This is done by using an object per name 
part. (A `NamePart` in a `SchemaPath`.) Here are we concatenating the names? If 
so, won't e have false matches?

```
['a', 'b.c'] --> "a.b.c"
['a.b', 'c'] --> "a.b.c"
```
The above are two distinct paths, but they will (wrongly) compare equals 
when concatenated.

Instead, do we want a list of name parts, and want to compare the names 
part by part? Does Calcite represent the path as a collection of objects? (If 
not, we've got larger problems...)


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r145565023
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SchemaUtilites.java
 ---
@@ -121,6 +137,9 @@ public static String getSchemaPath(List 
schemaPath) {
 return SCHEMA_PATH_JOINER.join(schemaPath);
   }
 
+  /** Utility method to get the schema path from one or more strings. */
+  public static String getSchemaPath(String s1, String s2, String... rest) 
{ return SCHEMA_PATH_JOINER.join(s1,s2,rest); }
--- End diff --

Can't join the names for reasons cited above.


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[paul-rogers]

Github user paul-rogers commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r145564522
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SchemaUtilites.java
 ---
@@ -77,6 +77,22 @@ public static SchemaPlus findSchema(final SchemaPlus 
defaultSchema, final String
 return findSchema(defaultSchema, schemaPathAsList);
   }
 
+  /**
+   * Utility function to get the commonPrefix schema between two supplied 
schemas.
+   * @param defaultSchema default schema
+   * @param schemaPath current schema path
+   * @param isCaseSensitive true if caseSensitive comparision is required.
+   * @return common prefix schemaPath
+   */
+  public static String getPrefixSchemaPath(final String defaultSchema, 
final String schemaPath, final boolean isCaseSensitive) {
--- End diff --

Isn't Drill always case insensitive since it follows SQL naming rules? 
We've often discussed how to handle case insensitive data sources, but we have 
no uniform, workable design. Is there a pattern we are following here?


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[HanumathRao]

Github user HanumathRao commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r145268123
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java
 ---
@@ -481,6 +485,19 @@ public RelOptTableImpl getTable(final List 
names) {
 .message("Temporary tables usage is disallowed. Used temporary 
table name: %s.", names)
 .build(logger);
   }
+
+  // Check the schema and throw a valid SchemaNotFound exception 
instead of TableNotFound exception.
--- End diff --

Thank you for the review. I agree that this should ideally be handled at 
Calcite layer. I also think that even after Calcite providing this 
functionality there should be some customization that needs to be done as we 
understand the context better than Calcite. Once the calcite fixes this issue 
then we can always change the code accordingly. 


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[arina-ielchiieva]

Github user arina-ielchiieva commented on a diff in the pull request:

https://github.com/apache/drill/pull/996#discussion_r145231215
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java
 ---
@@ -481,6 +485,19 @@ public RelOptTableImpl getTable(final List 
names) {
 .message("Temporary tables usage is disallowed. Used temporary 
table name: %s.", names)
 .build(logger);
   }
+
+  // Check the schema and throw a valid SchemaNotFound exception 
instead of TableNotFound exception.
--- End diff --

Does it mean that Calcite instead of returning schema not found exception 
returns table not found exception?
Per my understanding this PR customizes Drill but what if we go different 
path and enhance Calcite (or may be this is already done in newer Calcite 
versions)?


---

[GitHub] drill pull request #996: DRILL-5878: TableNotFound exception is being report...

[HanumathRao]

GitHub user HanumathRao opened a pull request:

https://github.com/apache/drill/pull/996

DRILL-5878: TableNotFound exception is being reported for a wrong sto…

…rage plugin.

@paul-rogers  @chunhui-shi Please review these changes. These changes are 
for reporting a meaningful error in case of wrong storage plugin is specified 
in the query. Currently Drill reports TableNotFound exception for
1) if no storage plugin exists.
2) if no table with the tablename exists.
These changes report SchemaNotFound exception for case 1) and TableNotFound 
exception for 2).

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/HanumathRao/drill DRILL-5878

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/996.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 #996


commit 1eba3a7234fbcf61659a1667aebc5034081b4984
Author: Hanumath Rao Maduri 
Date:   2017-09-16T23:54:00Z

DRILL-5878: TableNotFound exception is being reported for a wrong storage 
plugin.




---