Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4357: Fix DROP TABLE to pass analysis if the table fails 
to load
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5144/3/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
File fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java:

Line 105:       // we set it as TABLE as VIEW loading is unlikely to fail and 
even if it does
> What do you think about changing this to TCatalogObjectType.TABLE? We can a
Done


http://gerrit.cloudera.org:8080/#/c/5144/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

Line 1277:       } catch (NoSuchObjectException e) {
> This doesn't seem quite right. If we don't have IF EXISTS, then we should t
I don't quite understand how this is different from what the patch was 
originally trying to solve - making it possible to drop tables that were 
deleted externally in Kudu, except that its for tables deleted externally from 
Hive.

IF EXISTS would still apply, but only for whether or not a table exists in 
Impala's catalog cache, such that any table that is displayed in "SHOW TABLES" 
can be deleted without "IF EXISTS".

This also brings up the point that the patch only works for the original Kudu 
situation because we've hard-coded IF EXISTS to true for Kudu operations on 
line 1259 here.

I guess the answer to those concerns is that we're intentionally treating Hive 
and Kudu differently, since HMS is the ground truth of our catalog, though it 
seems inconsistent to me.

Either way, if I don't make this change another change is needed somewhere, as 
the AnalysisException that you used to get in the "externally deleted from 
Hive" situation hinted that "invalidate metadata" was the solution to the 
situation, but with this patch the ImpalaRuntimeException you get doesn't say 
that, which would seem to be a usability regression.


-- 
To view, visit http://gerrit.cloudera.org:8080/5144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b41fc3c0e95508ab67f1d420b033b02ec75a5da
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to