Hive
  1. Hive
  2. HIVE-1537

Allow users to specify LOCATION in CREATE DATABASE statement

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 0.8.0
    • Component/s: Metastore
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      Allow users to create database at a specific location. All tables created with that database will have their table-directory under the location.

      Usage:
      create database <dbname> location <path>;
      Show
      Allow users to create database at a specific location. All tables created with that database will have their table-directory under the location. Usage: create database <dbname> location <path>;
    • Tags:
      database
    1. hive-1537.metastore.part.patch
      2 kB
      Devaraj Das
    2. HIVE-1537.patch
      35 kB
      Thiruvel Thirumoolan
    3. HIVE-1537_3.patch
      30 kB
      Thiruvel Thirumoolan

      Issue Links

        Activity

        Hide
        Hudson added a comment -

        Integrated in Hive-trunk-h0.21 #820 (See https://builds.apache.org/job/Hive-trunk-h0.21/820/)
        HIVE-1537. Allow users to specify LOCATION in CREATE DATABASE statement. Contributed by Thiruvel Thirumoolan

        amareshwari : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1145053
        Files :

        • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java
        • /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java
        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
        • /hive/trunk/ql/src/test/results/clientpositive/database_location.q.out
        • /hive/trunk/ql/src/test/queries/clientpositive/database_location.q
        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java
        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java
        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g
        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java
        • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java
        • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
        Show
        Hudson added a comment - Integrated in Hive-trunk-h0.21 #820 (See https://builds.apache.org/job/Hive-trunk-h0.21/820/ ) HIVE-1537 . Allow users to specify LOCATION in CREATE DATABASE statement. Contributed by Thiruvel Thirumoolan amareshwari : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1145053 Files : /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java /hive/trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java /hive/trunk/ql/src/test/results/clientpositive/database_location.q.out /hive/trunk/ql/src/test/queries/clientpositive/database_location.q /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
        Hide
        Amareshwari Sriramadasu added a comment -

        I just committed this. Thanks Thiruvel !

        Show
        Amareshwari Sriramadasu added a comment - I just committed this. Thanks Thiruvel !
        Hide
        Amareshwari Sriramadasu added a comment -

        The tests drop_multi_partitions.q, escape1.q failed. The tests failed without the patch also. Will commit it now.

        Show
        Amareshwari Sriramadasu added a comment - The tests drop_multi_partitions.q, escape1.q failed. The tests failed without the patch also. Will commit it now.
        Hide
        Amareshwari Sriramadasu added a comment -

        +1
        Running tests.

        Show
        Amareshwari Sriramadasu added a comment - +1 Running tests.
        Hide
        Thiruvel Thirumoolan added a comment -
        Show
        Thiruvel Thirumoolan added a comment - Patch reviewed @ https://reviews.apache.org/r/949/diff/
        Hide
        Thiruvel Thirumoolan added a comment -

        Uploading patch that was reviewed.

        Show
        Thiruvel Thirumoolan added a comment - Uploading patch that was reviewed.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/#review966
        -----------------------------------------------------------

        Ship it!

        +1
        Looks good to me.

        • Ashutosh

        On 2011-07-06 12:14:34, Thiruvel Thirumoolan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/949/

        -----------------------------------------------------------

        (Updated 2011-07-06 12:14:34)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Summary

        -------

        Usage:

        create database location 'path1';

        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:

        ------

        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.

        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.

        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs

        -----

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1143322

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1143322

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1143322

        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1143322

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1143322

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1143322

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1143322

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1143322

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1143322

        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1143322

        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION

        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing

        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.

        2. Added database_location.q for testing the grammar primarily.

        Thanks,

        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/#review966 ----------------------------------------------------------- Ship it! +1 Looks good to me. Ashutosh On 2011-07-06 12:14:34, Thiruvel Thirumoolan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-07-06 12:14:34) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs ----- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1143322 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1143322 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1143322 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1143322 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1143322 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/
        -----------------------------------------------------------

        (Updated 2011-07-06 12:14:34.148278)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Changes
        -------

        Thanks Ashutosh, updated patch with your comments. Not sure whats the benefit of embedding the whole chunk inside transactions, so have not done it.

        Thanks!
        Thiruvel

        Summary
        -------

        Usage:

        create database location 'path1';
        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:
        ------
        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.
        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.
        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs (updated)


        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1143322
        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1143322
        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1143322
        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1143322
        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1143322
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1143322
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1143322
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1143322
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1143322
        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1143322
        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION
        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing
        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.
        2. Added database_location.q for testing the grammar primarily.

        Thanks,
        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-07-06 12:14:34.148278) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Changes ------- Thanks Ashutosh, updated patch with your comments. Not sure whats the benefit of embedding the whole chunk inside transactions, so have not done it. Thanks! Thiruvel Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs (updated) trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1143322 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1143322 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1143322 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1143322 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1143322 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1143322 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        Devaraj Das added a comment -

        I'm no longer at Yahoo!. My new email address is ddas@hortonworks.com. Please update your address book and resend your message.

        Show
        Devaraj Das added a comment - I'm no longer at Yahoo!. My new email address is ddas@hortonworks.com. Please update your address book and resend your message.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/#review959
        -----------------------------------------------------------

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
        <https://reviews.apache.org/r/949/#comment2036>

        If location already exists then create db will fail. This differs from create table semantics, where if location already exists then create table succeeds. I think we should follow create table in both semantics as well as code-wise, which does as follows:
        if (!wh.isDir(tblPath)) {
        if (!wh.mkdirs(tblPath))

        { throw new MetaException(tblPath + " is not a directory or unable to create one"); }

        madeDir = true;
        }

        Also note that fs operations are done within transaction.

        w.r.t deprecation Vs deletion of path api, I agree with Thiruvel's analysis, that its best to delete the api, since the old one will be buggy with these changes and also then there will be multiple ways to get path which will be confusing.

        • Ashutosh

        On 2011-06-29 11:17:50, Thiruvel Thirumoolan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/949/

        -----------------------------------------------------------

        (Updated 2011-06-29 11:17:50)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Summary

        -------

        Usage:

        create database location 'path1';

        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:

        ------

        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.

        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.

        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs

        -----

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1140495

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1140495

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1140495

        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1140495

        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1140495

        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION

        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing

        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.

        2. Added database_location.q for testing the grammar primarily.

        Thanks,

        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/#review959 ----------------------------------------------------------- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java < https://reviews.apache.org/r/949/#comment2036 > If location already exists then create db will fail. This differs from create table semantics, where if location already exists then create table succeeds. I think we should follow create table in both semantics as well as code-wise, which does as follows: if (!wh.isDir(tblPath)) { if (!wh.mkdirs(tblPath)) { throw new MetaException(tblPath + " is not a directory or unable to create one"); } madeDir = true; } Also note that fs operations are done within transaction. w.r.t deprecation Vs deletion of path api, I agree with Thiruvel's analysis, that its best to delete the api, since the old one will be buggy with these changes and also then there will be multiple ways to get path which will be confusing. Ashutosh On 2011-06-29 11:17:50, Thiruvel Thirumoolan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-06-29 11:17:50) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs ----- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1140495 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1140495 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1140495 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1140495 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1140495 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/#review930
        -----------------------------------------------------------

        Changes look fine to me.
        Ning, what do you think about removing the api in Warehouse.java vs deprecating them?

        • Amareshwari

        On 2011-06-29 11:17:50, Thiruvel Thirumoolan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/949/

        -----------------------------------------------------------

        (Updated 2011-06-29 11:17:50)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Summary

        -------

        Usage:

        create database location 'path1';

        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:

        ------

        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.

        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.

        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs

        -----

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1140495

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1140495

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1140495

        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1140495

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1140495

        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1140495

        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION

        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing

        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.

        2. Added database_location.q for testing the grammar primarily.

        Thanks,

        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/#review930 ----------------------------------------------------------- Changes look fine to me. Ning, what do you think about removing the api in Warehouse.java vs deprecating them? Amareshwari On 2011-06-29 11:17:50, Thiruvel Thirumoolan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-06-29 11:17:50) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs ----- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1140495 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1140495 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1140495 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1140495 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1140495 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/
        -----------------------------------------------------------

        (Updated 2011-06-29 11:17:50.481535)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Changes
        -------

        Updating old review request with the new patch, encountered an error yesterday and raised a new request.

        Changelog:

        1. Alter database has been removed.
        2. create database will fail when database directory cannot be created. (Devaraj's patch)

        Thanks,
        Thiruvel

        Summary
        -------

        Usage:

        create database location 'path1';
        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:
        ------
        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.
        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.
        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs (updated)


        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1140495
        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1140495
        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1140495
        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1140495
        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1140495
        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION
        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing
        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.
        2. Added database_location.q for testing the grammar primarily.

        Thanks,
        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-06-29 11:17:50.481535) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Changes ------- Updating old review request with the new patch, encountered an error yesterday and raised a new request. Changelog: 1. Alter database has been removed. 2. create database will fail when database directory cannot be created. (Devaraj's patch) Thanks, Thiruvel Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs (updated) trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1140495 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1140495 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1140495 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1140495 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1140495 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/973/
        -----------------------------------------------------------

        Review request for hive, Ashutosh Chauhan, Ning Zhang, and Amareshwari Sriramadasu.

        Summary
        -------

        • Removed alter database code from previous patch.
        • Updated create database to gracefully fail if database dir cannot be created (very close to Devaraj's patch)

        This addresses bug HIVE-1537.
        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs


        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1140495
        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1140495
        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1140495
        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1140495
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1140495
        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1140495
        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION
        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/973/diff

        Testing
        -------

        Updated testDatabaseLocation() in TestHiveMetaStore() to test for negative case.

        Ran entire unit test and it went through fine for me.

        Thanks,
        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/973/ ----------------------------------------------------------- Review request for hive, Ashutosh Chauhan, Ning Zhang, and Amareshwari Sriramadasu. Summary ------- Removed alter database code from previous patch. Updated create database to gracefully fail if database dir cannot be created (very close to Devaraj's patch) This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1140495 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1140495 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1140495 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1140495 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1140495 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1140495 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/973/diff Testing ------- Updated testDatabaseLocation() in TestHiveMetaStore() to test for negative case. Ran entire unit test and it went through fine for me. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        Thiruvel Thirumoolan added a comment -

        Will upload a revised patch without alter database.

        Show
        Thiruvel Thirumoolan added a comment - Will upload a revised patch without alter database.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-06-23 16:49:59, Ashutosh Chauhan wrote:

        > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line 591

        > <https://reviews.apache.org/r/949/diff/1/?file=21560#file21560line591>

        >

        > This may not be always successful. You may fail to create dirs for number of reasons. So, this needs to be handled gracefully. Transaction needs to rollback in such case and create database ddl needs to fail. For more info, look the first comment of Devaraj and also his attached partial patch.

        Thiruvel Thirumoolan wrote:

        I requested Devaraj offline to handle it in a separate JIRA. I am not sure about other methods having the same issue. That said, I introduced the same bug with alter_database. Will fix it for create and alter databases.

        Actually, problem exists in create Database even now without your patch. So, you are not making it any worse. I am fine if you prefer to address it in a followup jira.

        About alter database, I am not sure if there is any real usecase for it. Having a database spread across multiple locations is not a regular semantics. First concern is clean rollback semantics. Another is what about drop database in such scenarios, which directories are deleted when you drop a database, current one or all or one you specify in drop database ddl? You potentially need to persist all the locations of database in objectstore for deletion or for other purposes, which means a list of locationUri instead of a single string. Given all these, you might want to defer alter database to a new jira. Apart from better understanding of the usecases and semantics for alter database, doing it in two different jira will make this patch smaller and thus easier to get committed.

        • Ashutosh

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/#review898
        -----------------------------------------------------------

        On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/949/

        -----------------------------------------------------------

        (Updated 2011-06-23 09:55:50)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Summary

        -------

        Usage:

        create database location 'path1';

        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:

        ------

        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.

        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.

        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs

        -----

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011

        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011

        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011

        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION

        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing

        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.

        2. Added database_location.q for testing the grammar primarily.

        Thanks,

        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-06-23 16:49:59, Ashutosh Chauhan wrote: > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line 591 > < https://reviews.apache.org/r/949/diff/1/?file=21560#file21560line591 > > > This may not be always successful. You may fail to create dirs for number of reasons. So, this needs to be handled gracefully. Transaction needs to rollback in such case and create database ddl needs to fail. For more info, look the first comment of Devaraj and also his attached partial patch. Thiruvel Thirumoolan wrote: I requested Devaraj offline to handle it in a separate JIRA. I am not sure about other methods having the same issue. That said, I introduced the same bug with alter_database. Will fix it for create and alter databases. Actually, problem exists in create Database even now without your patch. So, you are not making it any worse. I am fine if you prefer to address it in a followup jira. About alter database, I am not sure if there is any real usecase for it. Having a database spread across multiple locations is not a regular semantics. First concern is clean rollback semantics. Another is what about drop database in such scenarios, which directories are deleted when you drop a database, current one or all or one you specify in drop database ddl? You potentially need to persist all the locations of database in objectstore for deletion or for other purposes, which means a list of locationUri instead of a single string. Given all these, you might want to defer alter database to a new jira. Apart from better understanding of the usecases and semantics for alter database, doing it in two different jira will make this patch smaller and thus easier to get committed. Ashutosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/#review898 ----------------------------------------------------------- On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-06-23 09:55:50) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs ----- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-06-24 06:32:41, Amareshwari Sriramadasu wrote:

        > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java, line 159

        > <https://reviews.apache.org/r/949/diff/1/?file=21562#file21562line159>

        >

        > I don't think we can just remove these public apis. Shall we deprecate them now and remove in next version?

        Dependence on dbname and not Database:

        If dbname is an arg, a Database object is required to get location and that will add a dependency to a RawStore instance (either creating it internally or as argument). The current separation of functionality seem to be breached and I did not want to do that. Let me know what you think.

        Making methods deprecated:

        0. I assumed this API is not used outside Hive and addressed internal usages. HCatalog will be inheriting this patch and that should work for them as well.
        1. Retaining the API without change would return wrong results for databases created with a location.
        2. Changing the API with change would add dependency to RawStore, which again I don't want.

        Considering all these, I removed it. Let me know if I am wrong.

        On 2011-06-24 06:32:41, Amareshwari Sriramadasu wrote:

        > trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g, line 613

        > <https://reviews.apache.org/r/949/diff/1/?file=21567#file21567line613>

        >

        > Similar to TOK_ALTERTABLE_*, we can still have TOK_ALTERDATABASE_DBPROPERTIES for properties and TOK_ALTERDATABASE_LOCATION for location. What do you think?

        I thought about it, I intended to keep it compact. The amount of work being done for each ALTERTABLE_* is high but its pretty small for database (at-least for now). But it does not hurt to separate out.

        • Thiruvel

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/#review905
        -----------------------------------------------------------

        On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/949/

        -----------------------------------------------------------

        (Updated 2011-06-23 09:55:50)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Summary

        -------

        Usage:

        create database location 'path1';

        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:

        ------

        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.

        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.

        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs

        -----

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011

        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011

        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011

        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION

        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing

        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.

        2. Added database_location.q for testing the grammar primarily.

        Thanks,

        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-06-24 06:32:41, Amareshwari Sriramadasu wrote: > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java, line 159 > < https://reviews.apache.org/r/949/diff/1/?file=21562#file21562line159 > > > I don't think we can just remove these public apis. Shall we deprecate them now and remove in next version? Dependence on dbname and not Database: If dbname is an arg, a Database object is required to get location and that will add a dependency to a RawStore instance (either creating it internally or as argument). The current separation of functionality seem to be breached and I did not want to do that. Let me know what you think. Making methods deprecated: 0. I assumed this API is not used outside Hive and addressed internal usages. HCatalog will be inheriting this patch and that should work for them as well. 1. Retaining the API without change would return wrong results for databases created with a location. 2. Changing the API with change would add dependency to RawStore, which again I don't want. Considering all these, I removed it. Let me know if I am wrong. On 2011-06-24 06:32:41, Amareshwari Sriramadasu wrote: > trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g, line 613 > < https://reviews.apache.org/r/949/diff/1/?file=21567#file21567line613 > > > Similar to TOK_ALTERTABLE_*, we can still have TOK_ALTERDATABASE_DBPROPERTIES for properties and TOK_ALTERDATABASE_LOCATION for location. What do you think? I thought about it, I intended to keep it compact. The amount of work being done for each ALTERTABLE_* is high but its pretty small for database (at-least for now). But it does not hurt to separate out. Thiruvel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/#review905 ----------------------------------------------------------- On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-06-23 09:55:50) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs ----- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-06-23 16:49:59, Ashutosh Chauhan wrote:

        > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line 591

        > <https://reviews.apache.org/r/949/diff/1/?file=21560#file21560line591>

        >

        > This may not be always successful. You may fail to create dirs for number of reasons. So, this needs to be handled gracefully. Transaction needs to rollback in such case and create database ddl needs to fail. For more info, look the first comment of Devaraj and also his attached partial patch.

        I requested Devaraj offline to handle it in a separate JIRA. I am not sure about other methods having the same issue. That said, I introduced the same bug with alter_database. Will fix it for create and alter databases.

        • Thiruvel

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/#review898
        -----------------------------------------------------------

        On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/949/

        -----------------------------------------------------------

        (Updated 2011-06-23 09:55:50)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Summary

        -------

        Usage:

        create database location 'path1';

        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:

        ------

        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.

        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.

        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs

        -----

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011

        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011

        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011

        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION

        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing

        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.

        2. Added database_location.q for testing the grammar primarily.

        Thanks,

        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-06-23 16:49:59, Ashutosh Chauhan wrote: > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, line 591 > < https://reviews.apache.org/r/949/diff/1/?file=21560#file21560line591 > > > This may not be always successful. You may fail to create dirs for number of reasons. So, this needs to be handled gracefully. Transaction needs to rollback in such case and create database ddl needs to fail. For more info, look the first comment of Devaraj and also his attached partial patch. I requested Devaraj offline to handle it in a separate JIRA. I am not sure about other methods having the same issue. That said, I introduced the same bug with alter_database. Will fix it for create and alter databases. Thiruvel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/#review898 ----------------------------------------------------------- On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-06-23 09:55:50) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs ----- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/#review905
        -----------------------------------------------------------

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java
        <https://reviews.apache.org/r/949/#comment1951>

        I don't think we can just remove these public apis. Shall we deprecate them now and remove in next version?

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java
        <https://reviews.apache.org/r/949/#comment1953>

        Again shall we deprecate this here?

        I would also be interested in an api getTablePath(dbName, tableName) i.e. without Database object.

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java
        <https://reviews.apache.org/r/949/#comment1954>

        Same as getTablePath

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g
        <https://reviews.apache.org/r/949/#comment1950>

        Similar to TOK_ALTERTABLE_*, we can still have TOK_ALTERDATABASE_DBPROPERTIES for properties and TOK_ALTERDATABASE_LOCATION for location. What do you think?

        • Amareshwari

        On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/949/

        -----------------------------------------------------------

        (Updated 2011-06-23 09:55:50)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Summary

        -------

        Usage:

        create database location 'path1';

        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:

        ------

        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.

        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.

        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs

        -----

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011

        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011

        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011

        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION

        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing

        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.

        2. Added database_location.q for testing the grammar primarily.

        Thanks,

        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/#review905 ----------------------------------------------------------- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java < https://reviews.apache.org/r/949/#comment1951 > I don't think we can just remove these public apis. Shall we deprecate them now and remove in next version? trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java < https://reviews.apache.org/r/949/#comment1953 > Again shall we deprecate this here? I would also be interested in an api getTablePath(dbName, tableName) i.e. without Database object. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java < https://reviews.apache.org/r/949/#comment1954 > Same as getTablePath trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g < https://reviews.apache.org/r/949/#comment1950 > Similar to TOK_ALTERTABLE_*, we can still have TOK_ALTERDATABASE_DBPROPERTIES for properties and TOK_ALTERDATABASE_LOCATION for location. What do you think? Amareshwari On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-06-23 09:55:50) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs ----- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/#review898
        -----------------------------------------------------------

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
        <https://reviews.apache.org/r/949/#comment1938>

        This may not be always successful. You may fail to create dirs for number of reasons. So, this needs to be handled gracefully. Transaction needs to rollback in such case and create database ddl needs to fail. For more info, look the first comment of Devaraj and also his attached partial patch.

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
        <https://reviews.apache.org/r/949/#comment1941>

        As previously, mkdirs() can fail, so handle similarly as in createDatabase()

        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java
        <https://reviews.apache.org/r/949/#comment1942>

        Please also add a test when a create database fails because a FS operation fails. In such a case no metadata should get created. One way to simulate that is to make location unwritable then try to create database on that location.

        • Ashutosh

        On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/949/

        -----------------------------------------------------------

        (Updated 2011-06-23 09:55:50)

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Summary

        -------

        Usage:

        create database location 'path1';

        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:

        ------

        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.

        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.

        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs

        -----

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011

        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011

        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011

        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011

        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011

        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION

        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing

        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.

        2. Added database_location.q for testing the grammar primarily.

        Thanks,

        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/#review898 ----------------------------------------------------------- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java < https://reviews.apache.org/r/949/#comment1938 > This may not be always successful. You may fail to create dirs for number of reasons. So, this needs to be handled gracefully. Transaction needs to rollback in such case and create database ddl needs to fail. For more info, look the first comment of Devaraj and also his attached partial patch. trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java < https://reviews.apache.org/r/949/#comment1941 > As previously, mkdirs() can fail, so handle similarly as in createDatabase() trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java < https://reviews.apache.org/r/949/#comment1942 > Please also add a test when a create database fails because a FS operation fails. In such a case no metadata should get created. One way to simulate that is to make location unwritable then try to create database on that location. Ashutosh On 2011-06-23 09:55:50, Thiruvel Thirumoolan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- (Updated 2011-06-23 09:55:50) Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs ----- trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/949/
        -----------------------------------------------------------

        Review request for hive, Ning Zhang and Amareshwari Sriramadasu.

        Summary
        -------

        Usage:

        create database location 'path1';
        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Notes:
        ------
        1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.
        2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever).

        This addresses bug HIVE-1537.
        https://issues.apache.org/jira/browse/HIVE-1537

        Diffs


        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011
        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011
        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011
        trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011
        trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011
        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011
        trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011
        trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011
        trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011
        trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION
        trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION

        Diff: https://reviews.apache.org/r/949/diff

        Testing
        -------

        1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations.
        2. Added database_location.q for testing the grammar primarily.

        Thanks,
        Thiruvel

        Thanks,

        Thiruvel

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/949/ ----------------------------------------------------------- Review request for hive, Ning Zhang and Amareshwari Sriramadasu. Summary ------- Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'. Notes: ------ 1. I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine. 2. One could argue why have getDatabasePath() as location can be obtained by db.getLocationUri(). I wanted to retain this method to do any additional processing if necessary (getDns or whatever). This addresses bug HIVE-1537 . https://issues.apache.org/jira/browse/HIVE-1537 Diffs trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java 1138011 trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java 1138011 trunk/metastore/src/test/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/StatsTask.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/DDLSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/Hive.g 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 1138011 trunk/ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzerFactory.java 1138011 trunk/ql/src/test/org/apache/hadoop/hive/ql/metadata/TestHive.java 1138011 trunk/ql/src/test/queries/clientpositive/database_location.q PRE-CREATION trunk/ql/src/test/results/clientpositive/database_location.q.out PRE-CREATION Diff: https://reviews.apache.org/r/949/diff Testing ------- 1. Updated TestHiveMetaStore.java for testing the functionality - database creation, alteration and table's locations as TestCliDriver outputs ignore locations. 2. Added database_location.q for testing the grammar primarily. Thanks, Thiruvel Thanks, Thiruvel
        Hide
        Thiruvel Thirumoolan added a comment -

        Notable changes:

        I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.

        Show
        Thiruvel Thirumoolan added a comment - Notable changes: I have moved getDefaultDatabasePath() to HiveMetaStore and made it private. There should only be one API to obtain the location of a database and it has to accept 'Database' as an arg and hence the new method in Warehouse 'getDatabasePath()' and similarly 'getTablePath()'. The usages of older API also has been changed. Hope that should be fine.
        Hide
        Thiruvel Thirumoolan added a comment -

        Usage:

        create database location 'path1';
        alter database location 'path2';

        After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.

        Show
        Thiruvel Thirumoolan added a comment - Usage: create database location 'path1'; alter database location 'path2'; After 'alter', only newly created tables will be located under the new location. Tables created before 'alter' will be under 'path1'.
        Hide
        Thiruvel Thirumoolan added a comment -

        @Bob will upload the patch sometime this week. Got busy with other stuff.

        Show
        Thiruvel Thirumoolan added a comment - @Bob will upload the patch sometime this week. Got busy with other stuff.
        Hide
        Bob Liu added a comment -

        Any idea as to when this feature will get implemented?

        Show
        Bob Liu added a comment - Any idea as to when this feature will get implemented?
        Hide
        Devaraj Das added a comment -

        Assuming that the location issue is solved, we need some checks in the create_database handler similar to what is there in the create_table handler.
        This patch is an example patch. It introduces a check in HiveMetaStore.create_database_core for existence of database directory, and also checks for failure to create one. In either case, the create_database_core operation throws an exception and the DDL would fail.

        Show
        Devaraj Das added a comment - Assuming that the location issue is solved, we need some checks in the create_database handler similar to what is there in the create_table handler. This patch is an example patch. It introduces a check in HiveMetaStore.create_database_core for existence of database directory, and also checks for failure to create one. In either case, the create_database_core operation throws an exception and the DDL would fail.

          People

          • Assignee:
            Thiruvel Thirumoolan
            Reporter:
            Carl Steinbach
          • Votes:
            1 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development