[
https://issues.apache.org/jira/browse/HIVE-23316?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17104314#comment-17104314
]
Miklos Gergely commented on HIVE-23316:
---------------------------------------
There is no issue, just that in DDL we have a clean model, and I believe that
there is an added value in following a model strictly, so one can be certain
that there is an Analyzer, Desc, and Operation for all the top level tokens.
Code reuse is a good thing, it is done all over DDL via utility classes, and
abstract ancestors for similar operations.
Before this modification AlterDatabaseSetLocationOperation contained the code
for both of the alter location commands, but I don't see any code reuse, the
upper part was for setting, location, the lower for setting managed location,
they were just put next to each other in the same class:
[https://github.com/apache/hive/commit/3ab174d82ffc2bd27432c0b04433be3bd7db5c6a#diff-f8c740f110b1d78ad04bf8f768504c45]
The two are very similar, there is a lot of duplication in them.
What I'd suggest though is to create only one command category from the two.
Instead of having the separate commands
{code:java}
ALTER DATABASE ... SET LOCATION ... -> TOK_ALTERDATABASE_LOCATION
ALTER DATABASE ... SET MANAGEDLOCATION ... ->
TOK_ALTERDATABASE_MANAGEDLOCATION{code}
Let's only have one command:
{code:java}
ALTER DATABASE ... SET [MANAGED] LOCATION ... -> TOK_ALTERDATABASE_LOCATION
{code}
Then the desc could have a
{code:java}
private final boolean isManaged;{code}
field, and only one set of classes is needed, with a lot of code reuse. Let me
know, if you agree.
> Add tests to cover database managed location related DDL and fix minor issues
> -----------------------------------------------------------------------------
>
> Key: HIVE-23316
> URL: https://issues.apache.org/jira/browse/HIVE-23316
> Project: Hive
> Issue Type: Bug
> Reporter: Miklos Gergely
> Assignee: Miklos Gergely
> Priority: Major
> Attachments: HIVE-23316.01.patch
>
>
> Database managed location was recently introduced, but no tests were added to
> cover it. also the following issues were fixed:
> * ALTER DATABASE ... SET MANAGEDLOCATION ... commands were not handled in a
> separate path as it should, as in DDL each command type have their own
> Analyzer, Desc, and Operation class
> * in case of setting the LOCATION or the MANAGEDLOCATION the location was
> not getting qualified as in the CREATE DATABASE command
> * in case of setting the LOCATION or the MANAGEDLOCATION it was not checked
> if this modification makes the two the same
> * some minor checkstyle issues were fixed as well
> Also the DDL documentation was not modified.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
