Thank you very much for taking a look at this patch!
Would we be better or returning CommandProcessorResponse's
Several of the execution methods will do further processing before invoking the processor so I don't feel we should invoke the processor and then return the result. Additionally in general it's common to use a factory to get the "executor" and then call the "executor" in the callers context as it sees fit. This approach fits that paradigm.
or throwing a new type of exception? I am struggling to rationalize SQLExceptions thrown from this part of hive code.
I certainly empathize with your feeling but I don't feel Hive we have the correct hierarchy to implement anything "better" at this point. In regards to throwing a SQLException I don't see what the advantage of throwing a separate exception would be. We are indicating the correct SQL State. But more importantly two callers are only going to log it regardless of the exception type and the third (HS2) is simply going to convert it to a HiveSQLException. I'd prefer to throw a HiveSQLException but that is a member of the service module and since service depends on ql it would create a circular dependency.
If at some point in the future we create additional layers of abstraction between the "tool" interface and the execution implementation then I could see an improved exception hierarchy. However, I don't feel that is a blocker for this patch. If there are concrete suggestions on this aspect I think it'd be a good use of a follow-on JIRA. The patch available at present improves admin's control, improves our switchboard logic, improves maintainability, and deletes almost as much code as it adds.