impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4357: Fix DROP TABLE to pass analysis if the table fails to load
Date Thu, 01 Dec 2016 23:19:48 GMT
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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message