Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.11.0
    • Component/s: Planner/Optimizer
    • Labels:
      None

      Description

      See the title. The main objective of this issue is to implement the logical planning part and the DDL executor part for alter table partition support.

      1. TAJO-1345_2.patch
        53 kB
        Jaehwa Jung
      2. TAJO-1345.patch
        30 kB
        Jaehwa Jung

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user blrunner opened a pull request:

        https://github.com/apache/tajo/pull/609

        TAJO-1345: Implement logical plan part and DDL executor for alter partition.

        See the title. The main objective of this issue is to implement the logical planning part and the DDL executor part for alter table partition support.

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/blrunner/tajo TAJO-1345

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/tajo/pull/609.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #609


        commit 7998019775de65ce6cebbd7453c0504375398b59
        Author: JaeHwa Jung <blrunner@apache.org>
        Date: 2015-06-22T10:41:13Z

        TAJO-1345: Implement logical plan part and DDL executor for alter partition.

        commit d90d5cdeea651d6296b60413f3894465ebd24a62
        Author: JaeHwa Jung <blrunner@apache.org>
        Date: 2015-06-22T10:44:24Z

        Remove unnecessary packages.


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user blrunner opened a pull request: https://github.com/apache/tajo/pull/609 TAJO-1345 : Implement logical plan part and DDL executor for alter partition. See the title. The main objective of this issue is to implement the logical planning part and the DDL executor part for alter table partition support. You can merge this pull request into a Git repository by running: $ git pull https://github.com/blrunner/tajo TAJO-1345 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/609.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #609 commit 7998019775de65ce6cebbd7453c0504375398b59 Author: JaeHwa Jung <blrunner@apache.org> Date: 2015-06-22T10:41:13Z TAJO-1345 : Implement logical plan part and DDL executor for alter partition. commit d90d5cdeea651d6296b60413f3894465ebd24a62 Author: JaeHwa Jung <blrunner@apache.org> Date: 2015-06-22T10:44:24Z Remove unnecessary packages.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner closed the pull request at:

        https://github.com/apache/tajo/pull/609

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner closed the pull request at: https://github.com/apache/tajo/pull/609
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user blrunner opened a pull request:

        https://github.com/apache/tajo/pull/618

        TAJO-1345: Implement logical plan part and DDL executor for alter partition.

        See the title. The main objective of this issue is to implement the logical planning part and the DDL executor part for alter table partition support.

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/blrunner/tajo TAJO-1345

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/tajo/pull/618.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #618


        commit 02a3c34008bcb40210746d226034e15fa3eff63e
        Author: JaeHwa Jung <blrunner@apache.org>
        Date: 2015-06-27T06:41:02Z

        TAJO-1345: Implement logical plan part and DDL executor for alter partition.


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user blrunner opened a pull request: https://github.com/apache/tajo/pull/618 TAJO-1345 : Implement logical plan part and DDL executor for alter partition. See the title. The main objective of this issue is to implement the logical planning part and the DDL executor part for alter table partition support. You can merge this pull request into a Git repository by running: $ git pull https://github.com/blrunner/tajo TAJO-1345 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/618.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #618 commit 02a3c34008bcb40210746d226034e15fa3eff63e Author: JaeHwa Jung <blrunner@apache.org> Date: 2015-06-27T06:41:02Z TAJO-1345 : Implement logical plan part and DDL executor for alter partition.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r33780996

        — Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java —
        @@ -348,7 +346,9 @@ public void alterTable(CatalogProtos.AlterTableDescProto alterTableDescProto) th
        if(!partitions.containsKey(tableName))

        { throw new NoSuchPartitionException(databaseName, tableName, partitionName); }

        else {

        • partitions.remove(partitionName);
          + Map<String, CatalogProtos.PartitionDescProto> protoMap = partitions.get(tableName);
          + protoMap.remove(partitionName);
          + partitions.put(tableName, protoMap);
            • End diff –

        Sorry. it's very trivial, but this line seems unnecessary.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r33780996 — Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java — @@ -348,7 +346,9 @@ public void alterTable(CatalogProtos.AlterTableDescProto alterTableDescProto) th if(!partitions.containsKey(tableName)) { throw new NoSuchPartitionException(databaseName, tableName, partitionName); } else { partitions.remove(partitionName); + Map<String, CatalogProtos.PartitionDescProto> protoMap = partitions.get(tableName); + protoMap.remove(partitionName); + partitions.put(tableName, protoMap); End diff – Sorry. it's very trivial, but this line seems unnecessary.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r33781510

        — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java —
        @@ -792,6 +794,40 @@ public static AlterTableDesc setProperty(String tableName, KeyValueSet params, A
        return alterTableDesc;
        }

        + public static AlterTableDesc addPartitionAndDropPartition(String tableName, String[] columns,
        — End diff –

        I think that the method name is too long. How about ```addOrDropPartition```? Or do you have any good name?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r33781510 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java — @@ -792,6 +794,40 @@ public static AlterTableDesc setProperty(String tableName, KeyValueSet params, A return alterTableDesc; } + public static AlterTableDesc addPartitionAndDropPartition(String tableName, String[] columns, — End diff – I think that the method name is too long. How about ```addOrDropPartition```? Or do you have any good name?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r33782109

        — Diff: tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java —
        @@ -446,11 +447,35 @@ public void alterTable(TajoMaster.MasterContext context, final QueryContext quer
        case SET_PROPERTY:
        catalog.alterTable(CatalogUtil.setProperty(qualifiedName, alterTable.getProperties(), AlterTableType.SET_PROPERTY));
        break;
        + case ADD_PARTITION:
        + existColumnNames(qualifiedName, alterTable.getColumnNames());
        + catalog.alterTable(CatalogUtil.addPartitionAndDropPartition(qualifiedName, alterTable.getColumnNames(),
        + alterTable.getPartitionValues(), alterTable.getLocation(), AlterTableType.ADD_PARTITION));
        + break;
        + case DROP_PARTITION:
        + existColumnNames(qualifiedName, alterTable.getColumnNames());
        + catalog.alterTable(CatalogUtil.addPartitionAndDropPartition(qualifiedName, alterTable.getColumnNames(),
        + alterTable.getPartitionValues(), alterTable.getLocation(), AlterTableType.DROP_PARTITION));
        + break;
        default:
        //TODO
        }
        }

        + private boolean existColumnNames(String tableName, String[] columnNames) {
        + for(String columnName : columnNames) {
        + if (!existPartitionColumnName(tableName, columnName))

        { + throw new NoSuchColumnException(columnName); + }

        + }
        + return true;
        + }
        +
        + private boolean existPartitionColumnName(String tableName, String columnName) {
        + final TableDesc tableDesc = catalog.getTableDesc(tableName);
        + return tableDesc.getPartitionMethod().getExpressionSchema().contains(columnName) ? true : false;
        — End diff –

        The ```?``` conditional operator seems unnecessary.
        It's also trivial, but makes an Intellij warning.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r33782109 — Diff: tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java — @@ -446,11 +447,35 @@ public void alterTable(TajoMaster.MasterContext context, final QueryContext quer case SET_PROPERTY: catalog.alterTable(CatalogUtil.setProperty(qualifiedName, alterTable.getProperties(), AlterTableType.SET_PROPERTY)); break; + case ADD_PARTITION: + existColumnNames(qualifiedName, alterTable.getColumnNames()); + catalog.alterTable(CatalogUtil.addPartitionAndDropPartition(qualifiedName, alterTable.getColumnNames(), + alterTable.getPartitionValues(), alterTable.getLocation(), AlterTableType.ADD_PARTITION)); + break; + case DROP_PARTITION: + existColumnNames(qualifiedName, alterTable.getColumnNames()); + catalog.alterTable(CatalogUtil.addPartitionAndDropPartition(qualifiedName, alterTable.getColumnNames(), + alterTable.getPartitionValues(), alterTable.getLocation(), AlterTableType.DROP_PARTITION)); + break; default: //TODO } } + private boolean existColumnNames(String tableName, String[] columnNames) { + for(String columnName : columnNames) { + if (!existPartitionColumnName(tableName, columnName)) { + throw new NoSuchColumnException(columnName); + } + } + return true; + } + + private boolean existPartitionColumnName(String tableName, String columnName) { + final TableDesc tableDesc = catalog.getTableDesc(tableName); + return tableDesc.getPartitionMethod().getExpressionSchema().contains(columnName) ? true : false; — End diff – The ```?``` conditional operator seems unnecessary. It's also trivial, but makes an Intellij warning.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r33783109

        — Diff: tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java —
        @@ -446,11 +447,35 @@ public void alterTable(TajoMaster.MasterContext context, final QueryContext quer
        case SET_PROPERTY:
        catalog.alterTable(CatalogUtil.setProperty(qualifiedName, alterTable.getProperties(), AlterTableType.SET_PROPERTY));
        break;
        + case ADD_PARTITION:
        + existColumnNames(qualifiedName, alterTable.getColumnNames());
        + catalog.alterTable(CatalogUtil.addPartitionAndDropPartition(qualifiedName, alterTable.getColumnNames(),
        + alterTable.getPartitionValues(), alterTable.getLocation(), AlterTableType.ADD_PARTITION));
        + break;
        + case DROP_PARTITION:
        + existColumnNames(qualifiedName, alterTable.getColumnNames());
        + catalog.alterTable(CatalogUtil.addPartitionAndDropPartition(qualifiedName, alterTable.getColumnNames(),
        + alterTable.getPartitionValues(), alterTable.getLocation(), AlterTableType.DROP_PARTITION));
        + break;
        default:
        //TODO
        }
        }

        + private boolean existColumnNames(String tableName, String[] columnNames) {
        + for(String columnName : columnNames) {
        + if (!existPartitionColumnName(tableName, columnName)) {
        + throw new NoSuchColumnException(columnName);
        — End diff –

        This case seems to indicate that the given table has a given column, but not as a partition key. If so, ```NoSuchColumnException``` makes users confused.
        How about make a new exception for it such as ```NoSuchPartitionKeyException```?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r33783109 — Diff: tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java — @@ -446,11 +447,35 @@ public void alterTable(TajoMaster.MasterContext context, final QueryContext quer case SET_PROPERTY: catalog.alterTable(CatalogUtil.setProperty(qualifiedName, alterTable.getProperties(), AlterTableType.SET_PROPERTY)); break; + case ADD_PARTITION: + existColumnNames(qualifiedName, alterTable.getColumnNames()); + catalog.alterTable(CatalogUtil.addPartitionAndDropPartition(qualifiedName, alterTable.getColumnNames(), + alterTable.getPartitionValues(), alterTable.getLocation(), AlterTableType.ADD_PARTITION)); + break; + case DROP_PARTITION: + existColumnNames(qualifiedName, alterTable.getColumnNames()); + catalog.alterTable(CatalogUtil.addPartitionAndDropPartition(qualifiedName, alterTable.getColumnNames(), + alterTable.getPartitionValues(), alterTable.getLocation(), AlterTableType.DROP_PARTITION)); + break; default: //TODO } } + private boolean existColumnNames(String tableName, String[] columnNames) { + for(String columnName : columnNames) { + if (!existPartitionColumnName(tableName, columnName)) { + throw new NoSuchColumnException(columnName); — End diff – This case seems to indicate that the given table has a given column, but not as a partition key. If so, ```NoSuchColumnException``` makes users confused. How about make a new exception for it such as ```NoSuchPartitionKeyException```?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r33784111

        — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java —
        @@ -42,6 +43,12 @@
        private KeyValueSet properties = new KeyValueSet();
        @Expose
        private AlterTableOpType alterTableOpType;
        + @Expose
        + private String[] columnNames;
        — End diff –

        ```colunmNames``` seems to mean partition columns, right? How about changing the name to be more intuitive?

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r33784111 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java — @@ -42,6 +43,12 @@ private KeyValueSet properties = new KeyValueSet(); @Expose private AlterTableOpType alterTableOpType; + @Expose + private String[] columnNames; — End diff – ```colunmNames``` seems to mean partition columns, right? How about changing the name to be more intuitive?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r33785085

        — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java —
        @@ -124,16 +155,18 @@ public PlanString getPlanString() {

        @Override
        public int hashCode() {

        • final int prime = 31;
        • int result = 1;
        • result = prime * result + ((addNewColumn == null) ? 0 : addNewColumn.hashCode());
        • result = prime * result + ((alterTableOpType == null) ? 0 : alterTableOpType.hashCode());
        • result = prime * result + ((columnName == null) ? 0 : columnName.hashCode());
        • result = prime * result + ((newColumnName == null) ? 0 : newColumnName.hashCode());
        • result = prime * result + ((newTableName == null) ? 0 : newTableName.hashCode());
        • result = prime * result + ((tableName == null) ? 0 : tableName.hashCode());
        • result = prime * result + ((properties == null) ? 0 : properties.hashCode());
        • return result;
          + return Objects.hashCode(tableName,
          + null != addNewColumn ? Objects.hashCode(addNewColumn) : addNewColumn,
            • End diff –

        Objects.hashCode() allows null parameters, so this null checking is unnecessary.
        Anyway, I think that the former way is more efficient than using Objects.hashCode().
        However, I also know that most hashCode() methods are written using Objects.hashCode(). So, we may need to improve those hashCode() methods when the performance issue arises later.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r33785085 — Diff: tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java — @@ -124,16 +155,18 @@ public PlanString getPlanString() { @Override public int hashCode() { final int prime = 31; int result = 1; result = prime * result + ((addNewColumn == null) ? 0 : addNewColumn.hashCode()); result = prime * result + ((alterTableOpType == null) ? 0 : alterTableOpType.hashCode()); result = prime * result + ((columnName == null) ? 0 : columnName.hashCode()); result = prime * result + ((newColumnName == null) ? 0 : newColumnName.hashCode()); result = prime * result + ((newTableName == null) ? 0 : newTableName.hashCode()); result = prime * result + ((tableName == null) ? 0 : tableName.hashCode()); result = prime * result + ((properties == null) ? 0 : properties.hashCode()); return result; + return Objects.hashCode(tableName, + null != addNewColumn ? Objects.hashCode(addNewColumn) : addNewColumn, End diff – Objects.hashCode() allows null parameters, so this null checking is unnecessary. Anyway, I think that the former way is more efficient than using Objects.hashCode(). However, I also know that most hashCode() methods are written using Objects.hashCode(). So, we may need to improve those hashCode() methods when the performance issue arises later.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-118058541

        Hi @blrunner, thank you for the patch. I left trivial comments only.

        In addition to them, I have one more comment.
        Add and drop partition statements seems to be allowed for only file based stores such as HDFS or local file system, right? If so, the partition location should be verified.
        Also, the existence of partition location should be checked.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-118058541 Hi @blrunner, thank you for the patch. I left trivial comments only. In addition to them, I have one more comment. Add and drop partition statements seems to be allowed for only file based stores such as HDFS or local file system, right? If so, the partition location should be verified. Also, the existence of partition location should be checked.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-118631499

        Hi @jihoonson

        Thanks for your detailed review. I updated the patch using your comments a few days ago.
        And I tested the partition location rule for hive. When adding partition on managed table or external table in hive, if the partition's directory doesn't exist, hive make the directory by force. And when dropping partition on managed table, hive will remove the directory by force, But when dropping partition on external table, hive will not remove the directory. If users want to remove the directory with drop partition statement, they need to specify PURGE option. It seems good to me because tajo partition had been made in reference to hive partition.

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-118631499 Hi @jihoonson Thanks for your detailed review. I updated the patch using your comments a few days ago. And I tested the partition location rule for hive. When adding partition on managed table or external table in hive, if the partition's directory doesn't exist, hive make the directory by force. And when dropping partition on managed table, hive will remove the directory by force, But when dropping partition on external table, hive will not remove the directory. If users want to remove the directory with drop partition statement, they need to specify PURGE option. It seems good to me because tajo partition had been made in reference to hive partition.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-118635039

        Hi @jihoonson

        I have updated the patch as following:

        • Added PURGE option.
        • When adding partition to a table, the partition's location will be made by force.
        • When dropping partition for a managed table, the location and meta data will be removed.
        • When dropping partition for an external table, the location will not be remove. Only meta data will be remove. If users want to remove the location, users need to specify PURGE option.
        • Added a document for alter table statement.

        Could you review the patch again?

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-118635039 Hi @jihoonson I have updated the patch as following: Added PURGE option. When adding partition to a table, the partition's location will be made by force. When dropping partition for a managed table, the location and meta data will be removed. When dropping partition for an external table, the location will not be remove. Only meta data will be remove. If users want to remove the location, users need to specify PURGE option. Added a document for alter table statement. Could you review the patch again?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-118757240

        @blrunner thanks for your nice work. I totally agree on that we follow Hive's partition policy.
        However, I found the invalid partition key generation error as follows.

        ```
        default> create table partitioned_nation (n_name text, n_comment text) partition by column (n_nationkey int8, n_regionkey int8) as select * from nation;

        default> \d partitioned_nation

        table name: default.partitioned_nation
        table uri: hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation
        store type: CSV
        number of rows: 25
        volume: 267 B
        Options:
        'text.delimiter'='|'

        schema:
        n_name TEXT
        n_comment TEXT

        Partitions:
        type:COLUMN
        columns::n_nationkey (INT8), n_regionkey (INT8)

        default> \dfs -ls hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0
        Found 5 items
        drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey= haggle. carefully final deposits detect slyly agai
        drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey= pending excuses haggle furiously deposits. pending, express pinto beans wake fluffily past t
        drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey=rns. blithely bold courts among the closely regular packages use furiously bold platelets%3F
        drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey=s. ironic, unusual asymptotes wake blithely r
        drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey=ven packages wake quickly. regu
        ```

        As you can see, the actual values of the partition key ```n_regionkey``` are those of ```n_comment``` column.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-118757240 @blrunner thanks for your nice work. I totally agree on that we follow Hive's partition policy. However, I found the invalid partition key generation error as follows. ``` default> create table partitioned_nation (n_name text, n_comment text) partition by column (n_nationkey int8, n_regionkey int8) as select * from nation; default> \d partitioned_nation table name: default.partitioned_nation table uri: hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation store type: CSV number of rows: 25 volume: 267 B Options: 'text.delimiter'='|' schema: n_name TEXT n_comment TEXT Partitions: type:COLUMN columns::n_nationkey (INT8), n_regionkey (INT8) default> \dfs -ls hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0 Found 5 items drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey= haggle. carefully final deposits detect slyly agai drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey= pending excuses haggle furiously deposits. pending, express pinto beans wake fluffily past t drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey=rns. blithely bold courts among the closely regular packages use furiously bold platelets%3F drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey=s. ironic, unusual asymptotes wake blithely r drwxr-xr-x - jihoonson supergroup 0 2015-07-06 16:29 hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/n_nationkey=0/n_regionkey=ven packages wake quickly. regu ``` As you can see, the actual values of the partition key ```n_regionkey``` are those of ```n_comment``` column.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-118763375

        This problem looks not to be related to this patch. I'll finish my review soon.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-118763375 This problem looks not to be related to this patch. I'll finish my review soon.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-118793127

        @blrunner, I have some more comments as follows.

        • The error message should be improved when the given column is not a partition key. Here is an example.
          ```
          default> alter table partitioned_nation add partition (n_comment=1, n_nationkey=2) location 'hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test/col2=2';
          ERROR: n_comment
          ```
        • I wonder that what will happen when an ```add partition``` statement is executed with the already existing location as follows.
        • The ```add partition``` statement does not create the full path of partition as follows.
          ```
          default> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test/col2=2';
          OK
          default> \dfs -ls hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test
          default>
          ```
        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-118793127 @blrunner, I have some more comments as follows. The error message should be improved when the given column is not a partition key. Here is an example. ``` default> alter table partitioned_nation add partition (n_comment=1, n_nationkey=2) location 'hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test/col2=2'; ERROR: n_comment ``` I wonder that what will happen when an ```add partition``` statement is executed with the already existing location as follows. The ```add partition``` statement does not create the full path of partition as follows. ``` default> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test/col2=2'; OK default> \dfs -ls hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test default> ```
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r33929768

        — Diff: tajo-core/src/test/resources/results/TestTajoCli/testAlterTableDropPartition.result —
        @@ -1,2 +1,2 @@
        OK
        -ERROR: DROP_PARTITION is not supported yet
        \ No newline at end of file
        +ERROR: java.lang.NullPointerException
        — End diff –

        It seems to be wrong.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r33929768 — Diff: tajo-core/src/test/resources/results/TestTajoCli/testAlterTableDropPartition.result — @@ -1,2 +1,2 @@ OK -ERROR: DROP_PARTITION is not supported yet \ No newline at end of file +ERROR: java.lang.NullPointerException — End diff – It seems to be wrong.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r33929844

        — Diff: tajo-docs/src/main/sphinx/sql_language/alter_table.rst —
        @@ -0,0 +1,99 @@
        +************************
        +ALTER TABLE Statement
        +************************
        +
        +========================
        +RENAME TABLE
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> RENAME TO <new_table_name>
        +
        + For example:
        + ALTER TABLE table1 RENAME TO table2;
        +
        +This statement lets you change the name of a table to a different name.
        +
        +========================
        +RENAME COLUMN
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> RENAME COLUMN <column_name> TO <new_column_name>
        +
        + For example:
        + ALTER TABLE table1 RENAME COLUMN id TO id2;
        +
        +This statement will allow users to change a column's name.
        +
        +========================
        +ADD COLUMN
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> ADD COLUMN <column_name> <data_type>
        +
        + For example:
        + ALTER TABLE table1 ADD COLUMN id text;
        +
        +This statement lets you add new columns to the end of the existing column.
        +
        +========================
        +SET PROPERTY
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> SET PROPERTY (<key> = <value>, ...)
        +
        + For example:
        + ALTER TABLE table1 SET PROPERTY 'timezone' = 'GMT-7'
        + ALTER TABLE table1 SET PROPERTY 'text.delimiter' = '&'
        + ALTER TABLE table1 SET PROPERTY 'compression.type'='RECORD','compression.codec'='org.apache.hadoop.io.compress.SnappyCodec'
        +
        +
        +This statement will allow users to change a table's properties.
        — End diff –

        'change a table property' would be better.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r33929844 — Diff: tajo-docs/src/main/sphinx/sql_language/alter_table.rst — @@ -0,0 +1,99 @@ +************************ +ALTER TABLE Statement +************************ + +======================== +RENAME TABLE +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> RENAME TO <new_table_name> + + For example: + ALTER TABLE table1 RENAME TO table2; + +This statement lets you change the name of a table to a different name. + +======================== +RENAME COLUMN +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> RENAME COLUMN <column_name> TO <new_column_name> + + For example: + ALTER TABLE table1 RENAME COLUMN id TO id2; + +This statement will allow users to change a column's name. + +======================== +ADD COLUMN +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> ADD COLUMN <column_name> <data_type> + + For example: + ALTER TABLE table1 ADD COLUMN id text; + +This statement lets you add new columns to the end of the existing column. + +======================== +SET PROPERTY +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> SET PROPERTY (<key> = <value>, ...) + + For example: + ALTER TABLE table1 SET PROPERTY 'timezone' = 'GMT-7' + ALTER TABLE table1 SET PROPERTY 'text.delimiter' = '&' + ALTER TABLE table1 SET PROPERTY 'compression.type'='RECORD','compression.codec'='org.apache.hadoop.io.compress.SnappyCodec' + + +This statement will allow users to change a table's properties. — End diff – 'change a table property' would be better.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r33930493

        — Diff: tajo-docs/src/main/sphinx/sql_language/alter_table.rst —
        @@ -0,0 +1,99 @@
        +************************
        +ALTER TABLE Statement
        +************************
        +
        +========================
        +RENAME TABLE
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> RENAME TO <new_table_name>
        +
        + For example:
        + ALTER TABLE table1 RENAME TO table2;
        +
        +This statement lets you change the name of a table to a different name.
        +
        +========================
        +RENAME COLUMN
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> RENAME COLUMN <column_name> TO <new_column_name>
        +
        + For example:
        + ALTER TABLE table1 RENAME COLUMN id TO id2;
        +
        +This statement will allow users to change a column's name.
        +
        +========================
        +ADD COLUMN
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> ADD COLUMN <column_name> <data_type>
        +
        + For example:
        + ALTER TABLE table1 ADD COLUMN id text;
        +
        +This statement lets you add new columns to the end of the existing column.
        +
        +========================
        +SET PROPERTY
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> SET PROPERTY (<key> = <value>, ...)
        +
        + For example:
        + ALTER TABLE table1 SET PROPERTY 'timezone' = 'GMT-7'
        + ALTER TABLE table1 SET PROPERTY 'text.delimiter' = '&'
        + ALTER TABLE table1 SET PROPERTY 'compression.type'='RECORD','compression.codec'='org.apache.hadoop.io.compress.SnappyCodec'
        +
        +
        +This statement will allow users to change a table's properties.
        +
        +========================
        +ADD PARTITION
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> [IF NOT EXISTS] ADD PARTITION (<partition column> = <partition value>, ...) [LOCATION = <partition's path>]
        +
        + For example:
        + ALTER TABLE table1 ADD PARTITION (col1 = 1 , col2 = 2)
        + ALTER TABLE table1 ADD PARTITION (col1 = 1 , col2 = 2) LOCATION 'hdfs://xxx.com/warehouse/table1/col1=1/col2=2'
        +
        +You can use ``ALTER TABLE ADD PARTITION`` to add partitions to a table. The location must be a directory inside of which data files reside. If the location doesn't exist on the file system, Tajo will make the location by force. ``ADD PARTITION`` changes the table metadata, but does not load data. If the data does not exist in the partition's location, queries will not return any results.
        +
        +========================
        + DROP PARTITION
        +========================
        +
        +Synopsis
        +
        +.. code-block:: sql
        +
        + ALTER TABLE <table_name> [IF NOT EXISTS] DROP PARTITION (<partition column> = <partition value>, ...) [PURGE]
        +
        + For example:
        + ALTER TABLE table1 DROP PARTITION (col1 = 1 , col2 = 2)
        + ALTER TABLE table1 DROP PARTITION (col1 = '2015' , col2 = '01', col3 = '11' )
        + ALTER TABLE table1 DROP PARTITION (col1 = 'TAJO' ) PURGE
        +
        +You can use ``ALTER TABLE DROP PARTITION`` to drop a partition for a table. This removes the data for a managed table and this doesn't remove the data for an external table. But if ``PURGE`` is specified for an external table, the partition data will be removed. For the reference, the metadata is completely lost in all cases.
        — End diff –

        the expression 'for the reference' seems to be not usual in English. Please check it.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r33930493 — Diff: tajo-docs/src/main/sphinx/sql_language/alter_table.rst — @@ -0,0 +1,99 @@ +************************ +ALTER TABLE Statement +************************ + +======================== +RENAME TABLE +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> RENAME TO <new_table_name> + + For example: + ALTER TABLE table1 RENAME TO table2; + +This statement lets you change the name of a table to a different name. + +======================== +RENAME COLUMN +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> RENAME COLUMN <column_name> TO <new_column_name> + + For example: + ALTER TABLE table1 RENAME COLUMN id TO id2; + +This statement will allow users to change a column's name. + +======================== +ADD COLUMN +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> ADD COLUMN <column_name> <data_type> + + For example: + ALTER TABLE table1 ADD COLUMN id text; + +This statement lets you add new columns to the end of the existing column. + +======================== +SET PROPERTY +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> SET PROPERTY (<key> = <value>, ...) + + For example: + ALTER TABLE table1 SET PROPERTY 'timezone' = 'GMT-7' + ALTER TABLE table1 SET PROPERTY 'text.delimiter' = '&' + ALTER TABLE table1 SET PROPERTY 'compression.type'='RECORD','compression.codec'='org.apache.hadoop.io.compress.SnappyCodec' + + +This statement will allow users to change a table's properties. + +======================== +ADD PARTITION +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> [IF NOT EXISTS] ADD PARTITION (<partition column> = <partition value>, ...) [LOCATION = <partition's path>] + + For example: + ALTER TABLE table1 ADD PARTITION (col1 = 1 , col2 = 2) + ALTER TABLE table1 ADD PARTITION (col1 = 1 , col2 = 2) LOCATION 'hdfs://xxx.com/warehouse/table1/col1=1/col2=2' + +You can use ``ALTER TABLE ADD PARTITION`` to add partitions to a table. The location must be a directory inside of which data files reside. If the location doesn't exist on the file system, Tajo will make the location by force. ``ADD PARTITION`` changes the table metadata, but does not load data. If the data does not exist in the partition's location, queries will not return any results. + +======================== + DROP PARTITION +======================== + + Synopsis + +.. code-block:: sql + + ALTER TABLE <table_name> [IF NOT EXISTS] DROP PARTITION (<partition column> = <partition value>, ...) [PURGE] + + For example: + ALTER TABLE table1 DROP PARTITION (col1 = 1 , col2 = 2) + ALTER TABLE table1 DROP PARTITION (col1 = '2015' , col2 = '01', col3 = '11' ) + ALTER TABLE table1 DROP PARTITION (col1 = 'TAJO' ) PURGE + +You can use ``ALTER TABLE DROP PARTITION`` to drop a partition for a table. This removes the data for a managed table and this doesn't remove the data for an external table. But if ``PURGE`` is specified for an external table, the partition data will be removed. For the reference, the metadata is completely lost in all cases. — End diff – the expression 'for the reference' seems to be not usual in English. Please check it.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-119035338

        Thanks @hyunsik .
        I've updated the document.

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-119035338 Thanks @hyunsik . I've updated the document.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-119121857

        Thanks @jihoonson and @hyunsik.
        I've update the patch using your comments.

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-119121857 Thanks @jihoonson and @hyunsik. I've update the patch using your comments.
        Hide
        blrunner Jaehwa Jung added a comment -

        I've uploaded the latest version for build test.

        Show
        blrunner Jaehwa Jung added a comment - I've uploaded the latest version for build test.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r34547821

        — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionException.java —
        @@ -32,6 +32,10 @@ public NoSuchPartitionException(String message)

        { super(message); }

        + public NoSuchPartitionException(String tableName, String partitionName) {
        — End diff –

        The imports in this source code are unused. Please remove them. I know that this is not related to your changes.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r34547821 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionException.java — @@ -32,6 +32,10 @@ public NoSuchPartitionException(String message) { super(message); } + public NoSuchPartitionException(String tableName, String partitionName) { — End diff – The imports in this source code are unused. Please remove them. I know that this is not related to your changes.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r34547834

        — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java —
        @@ -0,0 +1,44 @@
        +/**
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +
        +package org.apache.tajo.catalog.exception;
        +
        +import org.apache.tajo.common.TajoDataTypes;
        — End diff –

        unused imports

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r34547834 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java — @@ -0,0 +1,44 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.tajo.catalog.exception; + +import org.apache.tajo.common.TajoDataTypes; — End diff – unused imports
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r34547863

        — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java —
        @@ -0,0 +1,44 @@
        +/**
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +
        +package org.apache.tajo.catalog.exception;
        +
        +import org.apache.tajo.common.TajoDataTypes;
        +import org.apache.tajo.function.FunctionUtil;
        +import org.codehaus.jackson.schema.JsonSerializableSchema;
        +
        +import java.util.Collection;
        +
        +public class NoSuchPartitionKeyException extends RuntimeException {
        +
        + private static final long serialVersionUID = 277182608283894939L;
        +
        + public NoSuchPartitionKeyException(String message) {
        — End diff –

        Please remove unused constructor.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r34547863 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java — @@ -0,0 +1,44 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.tajo.catalog.exception; + +import org.apache.tajo.common.TajoDataTypes; +import org.apache.tajo.function.FunctionUtil; +import org.codehaus.jackson.schema.JsonSerializableSchema; + +import java.util.Collection; + +public class NoSuchPartitionKeyException extends RuntimeException { + + private static final long serialVersionUID = 277182608283894939L; + + public NoSuchPartitionKeyException(String message) { — End diff – Please remove unused constructor.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r34547868

        — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java —
        @@ -0,0 +1,44 @@
        +/**
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +
        +package org.apache.tajo.catalog.exception;
        +
        +import org.apache.tajo.common.TajoDataTypes;
        +import org.apache.tajo.function.FunctionUtil;
        +import org.codehaus.jackson.schema.JsonSerializableSchema;
        +
        +import java.util.Collection;
        +
        +public class NoSuchPartitionKeyException extends RuntimeException {
        +
        + private static final long serialVersionUID = 277182608283894939L;
        +
        + public NoSuchPartitionKeyException(String message)

        { + super(message); + }

        +
        + public NoSuchPartitionKeyException(String tableName, String partitionKey)

        { + super(String.format("ERROR: partition column \"%s\" does not exist in \"%s\".", partitionKey, tableName)); + }

        +
        + public NoSuchPartitionKeyException(String databaseName, String tableName, String partitionKey) {
        — End diff –

        Please remove unused constructor.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r34547868 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java — @@ -0,0 +1,44 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.tajo.catalog.exception; + +import org.apache.tajo.common.TajoDataTypes; +import org.apache.tajo.function.FunctionUtil; +import org.codehaus.jackson.schema.JsonSerializableSchema; + +import java.util.Collection; + +public class NoSuchPartitionKeyException extends RuntimeException { + + private static final long serialVersionUID = 277182608283894939L; + + public NoSuchPartitionKeyException(String message) { + super(message); + } + + public NoSuchPartitionKeyException(String tableName, String partitionKey) { + super(String.format("ERROR: partition column \"%s\" does not exist in \"%s\".", partitionKey, tableName)); + } + + public NoSuchPartitionKeyException(String databaseName, String tableName, String partitionKey) { — End diff – Please remove unused constructor.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r34547955

        — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java —
        @@ -792,6 +791,69 @@ public static AlterTableDesc setProperty(String tableName, KeyValueSet params, A
        return alterTableDesc;
        }

        + /**
        + * Add partition or Drop partition
        — End diff –

        The comment should be improved because this method create AlterTableDesc instance rather than adding or dropping any partition.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r34547955 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java — @@ -792,6 +791,69 @@ public static AlterTableDesc setProperty(String tableName, KeyValueSet params, A return alterTableDesc; } + /** + * Add partition or Drop partition — End diff – The comment should be improved because this method create AlterTableDesc instance rather than adding or dropping any partition.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r34549680

        — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java —
        @@ -792,6 +791,69 @@ public static AlterTableDesc setProperty(String tableName, KeyValueSet params, A
        return alterTableDesc;
        }

        + /**
        + * Add partition or Drop partition
        + *
        + * @param tableName table name
        + * @param columns partition column names
        + * @param values partition values
        + * @param location partition location
        + * @param alterTableType ADD_PARTITION or DROP_PARTITION
        + * @return
        + */
        + public static AlterTableDesc addOrDropPartition(String tableName, String[] columns,
        + String[] values, String location, AlterTableType alterTableType) {
        + final AlterTableDesc alterTableDesc = new AlterTableDesc();
        + alterTableDesc.setTableName(tableName);
        +
        + PartitionDesc partitionDesc = new PartitionDesc();
        + Pair<List<PartitionKey>, String> pair = getPartitionKeyNamePair(columns, values);
        +
        + partitionDesc.setPartitionKeys(pair.getFirst());
        + partitionDesc.setPartitionName(pair.getSecond());
        +
        + if (alterTableType.equals(AlterTableType.ADD_PARTITION) && location != null)

        { + partitionDesc.setPath(location); + }

        +
        + alterTableDesc.setPartitionDesc(partitionDesc);
        + alterTableDesc.setAlterTableType(alterTableType);
        + return alterTableDesc;
        + }
        +
        + /**
        + * Get partition key/value list and partition name
        + *
        + * ex) partition key/value list :
        + * - col1, 2015-07-01
        + * - col2, tajo
        + * partition name : col1=2015-07-01/col2=tajo
        + *
        + * @param columns partition column names
        + * @param values partition values
        + * @return partition key/value list and partition name
        + */
        + public static Pair<List<PartitionKey>, String> getPartitionKeyNamePair(String[] columns, String[] values) {
        + Pair<List<PartitionKey>, String> pair = null;
        + List<PartitionKey> partitionKeyList = TUtil.newList();
        +
        + StringBuilder sb = new StringBuilder();
        + for (int i = 0; i < columns.length; i++) {
        + PartitionKey partitionKey = new PartitionKey();
        — End diff –

        PartitionKey is used as transient intermediate data for PartitionDesc. I think that PartitionKey is unnecessary. You may use directly PartitionKeyProto without PartitionKey.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r34549680 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java — @@ -792,6 +791,69 @@ public static AlterTableDesc setProperty(String tableName, KeyValueSet params, A return alterTableDesc; } + /** + * Add partition or Drop partition + * + * @param tableName table name + * @param columns partition column names + * @param values partition values + * @param location partition location + * @param alterTableType ADD_PARTITION or DROP_PARTITION + * @return + */ + public static AlterTableDesc addOrDropPartition(String tableName, String[] columns, + String[] values, String location, AlterTableType alterTableType) { + final AlterTableDesc alterTableDesc = new AlterTableDesc(); + alterTableDesc.setTableName(tableName); + + PartitionDesc partitionDesc = new PartitionDesc(); + Pair<List<PartitionKey>, String> pair = getPartitionKeyNamePair(columns, values); + + partitionDesc.setPartitionKeys(pair.getFirst()); + partitionDesc.setPartitionName(pair.getSecond()); + + if (alterTableType.equals(AlterTableType.ADD_PARTITION) && location != null) { + partitionDesc.setPath(location); + } + + alterTableDesc.setPartitionDesc(partitionDesc); + alterTableDesc.setAlterTableType(alterTableType); + return alterTableDesc; + } + + /** + * Get partition key/value list and partition name + * + * ex) partition key/value list : + * - col1, 2015-07-01 + * - col2, tajo + * partition name : col1=2015-07-01/col2=tajo + * + * @param columns partition column names + * @param values partition values + * @return partition key/value list and partition name + */ + public static Pair<List<PartitionKey>, String> getPartitionKeyNamePair(String[] columns, String[] values) { + Pair<List<PartitionKey>, String> pair = null; + List<PartitionKey> partitionKeyList = TUtil.newList(); + + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < columns.length; i++) { + PartitionKey partitionKey = new PartitionKey(); — End diff – PartitionKey is used as transient intermediate data for PartitionDesc. I think that PartitionKey is unnecessary. You may use directly PartitionKeyProto without PartitionKey.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-121178043

        In overall, the patch looks good to me. I leaved few comments.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-121178043 In overall, the patch looks good to me. I leaved few comments.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-121264989

        @hyunsik

        Thank you for your detailed review.
        I've updated the patch using your comments.

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-121264989 @hyunsik Thank you for your detailed review. I've updated the patch using your comments.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on a diff in the pull request:

        https://github.com/apache/tajo/pull/618#discussion_r34579447

        — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java —
        @@ -0,0 +1,28 @@
        +/**
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +
        +package org.apache.tajo.catalog.exception;
        +
        +public class NoSuchPartitionKeyException extends RuntimeException {
        +
        + private static final long serialVersionUID = 277182608283894939L;
        +
        + public NoSuchPartitionKeyException(String tableName, String partitionKey) {
        + super(String.format("ERROR: partition column \"%s\" does not exist in \"%s\".", partitionKey, tableName));
        — End diff –

        This message means that the table of ```tableName``` does not have the ```partitionKey``` column.
        It would be better to print the ```partitionKey``` column is not the partition key of the ```tableName``` table.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on a diff in the pull request: https://github.com/apache/tajo/pull/618#discussion_r34579447 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java — @@ -0,0 +1,28 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.tajo.catalog.exception; + +public class NoSuchPartitionKeyException extends RuntimeException { + + private static final long serialVersionUID = 277182608283894939L; + + public NoSuchPartitionKeyException(String tableName, String partitionKey) { + super(String.format("ERROR: partition column \"%s\" does not exist in \"%s\".", partitionKey, tableName)); — End diff – This message means that the table of ```tableName``` does not have the ```partitionKey``` column. It would be better to print the ```partitionKey``` column is not the partition key of the ```tableName``` table.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-121286848

        It seems that there is another problem. That is, given a partitioned table, ```alter table add partition``` is executed in two ways.
        Here is an example.

        ```
        tpch> create table partitioned_nation (n_name text, n_comment text) using text partition by column (n_regionkey int8, n_nationkey int8) as select n_name, n_comment, n_regionkey, n_nationkey from nation;
        tpch> \dfs -ls /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1
        Found 5 items
        drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=1
        drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=17
        drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=2
        drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=24
        drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=3
        ```
        As you can see in the above, the partition of (n_regionkey=1,n_nationkey=2) exists after creating the partition table.
        When I run the following ```alter table add partition``` statement, it is successfully executed. However, I think that this should be failed because there already exists a partition for the same partition key.

        ```
        tpch> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test2/col2=3';
        OK
        ```
        In addition, when I run the following ```alter table add partition``` statement, it is failed as I expected above. But, the inconsistent execution of ```alter table add partition``` must be fixed.
        ```
        tpch> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test1/col2=2';
        ERROR: partition "n_regionkey=1/n_nationkey=2 already exist in "partitioned_nation"
        ```

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-121286848 It seems that there is another problem. That is, given a partitioned table, ```alter table add partition``` is executed in two ways. Here is an example. ``` tpch> create table partitioned_nation (n_name text, n_comment text) using text partition by column (n_regionkey int8, n_nationkey int8) as select n_name, n_comment, n_regionkey, n_nationkey from nation; tpch> \dfs -ls /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1 Found 5 items drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=1 drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=17 drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=2 drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=24 drwxr-xr-x - jihoon supergroup 0 2015-07-15 00:22 /tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=3 ``` As you can see in the above, the partition of (n_regionkey=1,n_nationkey=2) exists after creating the partition table. When I run the following ```alter table add partition``` statement, it is successfully executed. However, I think that this should be failed because there already exists a partition for the same partition key. ``` tpch> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test2/col2=3'; OK ``` In addition, when I run the following ```alter table add partition``` statement, it is failed as I expected above. But, the inconsistent execution of ```alter table add partition``` must be fixed. ``` tpch> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:7020/tajo/warehouse/default/partitioned_nation/col1=test1/col2=2'; ERROR: partition "n_regionkey=1/n_nationkey=2 already exist in "partitioned_nation" ```
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-121430758

        Hi @jihoonson

        Thank you for your detailed test. I also think that it looks like a problem. But currently, when creating partitions using DDL, tajo just make partitioned directories and doesn't add partitions to catalog. But when executing alter table add partition, this statement just check partitions from catalog. Thus it doesn't know partitions which are generated by DDL. This issue would be resolved automatically at TAJO-1346.

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-121430758 Hi @jihoonson Thank you for your detailed test. I also think that it looks like a problem. But currently, when creating partitions using DDL, tajo just make partitioned directories and doesn't add partitions to catalog. But when executing alter table add partition, this statement just check partitions from catalog. Thus it doesn't know partitions which are generated by DDL. This issue would be resolved automatically at TAJO-1346 .
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-121431232

        Ok. But the last query is failed even though the location is different.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-121431232 Ok. But the last query is failed even though the location is different.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-121437463

        Catalog check the identity of partition using the name of partition instead of location. In above case, the name of partition is ``n_regionkey=1,n_nationkey=2``. If you didn't execute ``alter table drop partition statement``, the partition of (n_regionkey=1,n_nationkey=2) exists on catalog. Could you execute ``alter table add partition statement`` after executing ``alter table drop partition statement``?

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-121437463 Catalog check the identity of partition using the name of partition instead of location. In above case, the name of partition is ``n_regionkey=1,n_nationkey=2``. If you didn't execute ``alter table drop partition statement``, the partition of (n_regionkey=1,n_nationkey=2) exists on catalog. Could you execute ``alter table add partition statement`` after executing ``alter table drop partition statement``?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-121447644

        Sorry @jihoonson

        I misunderstood the question and will add the exception for this case.

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-121447644 Sorry @jihoonson I misunderstood the question and will add the exception for this case.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-121523880

        @jihoonson

        I added the exception for above case.
        If there is a directory which is assumed to be a partitioned directory, tajo will throw exception as follows.

        ```
        default> create table partitioned_nation (n_name text, n_comment text) using text partition by column (n_regionkey int8, n_nationkey int8) as select n_name, n_comment, n_regionkey, n_nationkey from nation;

        default> \dfs -ls /tajo/warehouse/default/partitioned_nation/n_regionkey=1
        Found 5 items
        drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=1
        drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=17
        drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=2
        drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=24
        drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=3

        default> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:9010/tajo/warehouse/default/partitioned_nation/col1=test2/col2=3';
        ERROR: There is a directory which is assumed to be a partitioned directory : hdfs://localhost:9010/tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=2

        default> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:9010/tajo/warehouse/default/partitioned_nation/col1=test1/col2=2';
        ERROR: There is a directory which is assumed to be a partitioned directory : hdfs://localhost:9010/tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=2
        ```

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-121523880 @jihoonson I added the exception for above case. If there is a directory which is assumed to be a partitioned directory, tajo will throw exception as follows. ``` default> create table partitioned_nation (n_name text, n_comment text) using text partition by column (n_regionkey int8, n_nationkey int8) as select n_name, n_comment, n_regionkey, n_nationkey from nation; default> \dfs -ls /tajo/warehouse/default/partitioned_nation/n_regionkey=1 Found 5 items drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=1 drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=17 drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=2 drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=24 drwxr-xr-x - blrunner supergroup 0 2015-07-15 16:21 /tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=3 default> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:9010/tajo/warehouse/default/partitioned_nation/col1=test2/col2=3'; ERROR: There is a directory which is assumed to be a partitioned directory : hdfs://localhost:9010/tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=2 default> alter table partitioned_nation add partition (n_regionkey=1, n_nationkey=2) location 'hdfs://localhost:9010/tajo/warehouse/default/partitioned_nation/col1=test1/col2=2'; ERROR: There is a directory which is assumed to be a partitioned directory : hdfs://localhost:9010/tajo/warehouse/default/partitioned_nation/n_regionkey=1/n_nationkey=2 ```
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-122144338

        @blrunner thanks for update.
        However, there still remains a similar problem when dropping a partition. That is, newly added partitions with ```alter table``` statement are successfully dropped, but others are not.
        Here is an example.
        ```
        tpch> \d partitioned_nation

        table name: tpch.partitioned_nation
        table uri: hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation
        store type: text
        number of rows: 25
        volume: 2.1 kB
        Options:
        'text.delimiter'='|'

        schema:
        n_name TEXT
        n_comment TEXT

        Partitions:
        type:COLUMN
        columns::n_regionkey (INT8), n_nationkey (INT8)

        tpch> \dfs -ls hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1
        Found 5 items
        drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=1
        drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=17
        drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=2
        drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=24
        drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=3

        tpch> alter table partitioned_nation drop partition (n_regionkey=1, n_nationkey=2);
        ERROR: "n_regionkey=1/n_nationkey=2" is not the partition of "partitioned_nation".
        tpch> alter table partitioned_nation drop partition (n_regionkey=10, n_nationkey=2);
        OK
        ```
        As you can see in this example, the partition of (n_regionkey=1, n_nationkey=2) exists, but dropping it is failed.

        The fundamental problem seems that partition information is not maintained by catalog. I believe that this will be resolved in https://issues.apache.org/jira/browse/TAJO-1346, and thus the above problem will be resolved, too.
        So, I'll give +1. Thanks for your long time work.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-122144338 @blrunner thanks for update. However, there still remains a similar problem when dropping a partition. That is, newly added partitions with ```alter table``` statement are successfully dropped, but others are not. Here is an example. ``` tpch> \d partitioned_nation table name: tpch.partitioned_nation table uri: hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation store type: text number of rows: 25 volume: 2.1 kB Options: 'text.delimiter'='|' schema: n_name TEXT n_comment TEXT Partitions: type:COLUMN columns::n_regionkey (INT8), n_nationkey (INT8) tpch> \dfs -ls hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1 Found 5 items drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=1 drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=17 drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=2 drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=24 drwxr-xr-x - jihoon supergroup 0 2015-07-17 10:20 hdfs://localhost:7020/tajo/warehouse/warehouse/tpch/partitioned_nation/n_regionkey=1/n_nationkey=3 tpch> alter table partitioned_nation drop partition (n_regionkey=1, n_nationkey=2); ERROR: "n_regionkey=1/n_nationkey=2" is not the partition of "partitioned_nation". tpch> alter table partitioned_nation drop partition (n_regionkey=10, n_nationkey=2); OK ``` As you can see in this example, the partition of (n_regionkey=1, n_nationkey=2) exists, but dropping it is failed. The fundamental problem seems that partition information is not maintained by catalog. I believe that this will be resolved in https://issues.apache.org/jira/browse/TAJO-1346 , and thus the above problem will be resolved, too. So, I'll give +1. Thanks for your long time work.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user blrunner commented on the pull request:

        https://github.com/apache/tajo/pull/618#issuecomment-122178566

        Hi @jihoonson

        Thanks for your review of great depth.
        As you commented, above case will be resolved automatically in TAJO-1346. I'll commit it to the master branch soon.

        Show
        githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/618#issuecomment-122178566 Hi @jihoonson Thanks for your review of great depth. As you commented, above case will be resolved automatically in TAJO-1346 . I'll commit it to the master branch soon.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

        https://github.com/apache/tajo/pull/618

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tajo/pull/618
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Tajo-master-build #750 (See https://builds.apache.org/job/Tajo-master-build/750/)
        TAJO-1345: Implement logical plan part and DDL executor for alter partition. (jaehwa) (blrunner: rev 3dba8c7e6bc539b07b136b959aa91dc4402be380)

        • tajo-core/src/test/resources/queries/TestAlterTable/alter_table_add_partition1.sql
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/partition/PartitionKey.java
        • tajo-catalog/tajo-catalog-drivers/tajo-hive/src/test/java/org/apache/tajo/catalog/store/TestHiveCatalogStore.java
        • tajo-core/src/test/java/org/apache/tajo/engine/planner/TestLogicalPlanner.java
        • tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java
        • tajo-docs/src/main/sphinx/sql_language.rst
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/partition/PartitionDesc.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/AlreadyExistsPartitionException.java
        • tajo-core/src/test/resources/queries/TestAlterTable/create_partitioned_table.sql
        • tajo-plan/src/main/java/org/apache/tajo/plan/verifier/PreLogicalPlanVerifier.java
        • tajo-core/src/test/resources/results/TestTajoCli/testAlterTableDropPartition.result
        • tajo-core/src/test/java/org/apache/tajo/jdbc/TestTajoJdbc.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoPartitionedTableException.java
        • tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeSerializer.java
        • tajo-core/src/test/resources/results/TestTajoCli/testAlterTableAddDropPartition.result
        • tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeDeserializer.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java
        • CHANGES
        • tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java
        • tajo-plan/src/main/proto/Plan.proto
        • tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java
        • tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java
        • tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java
        • tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java
        • tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4
        • tajo-core/src/test/resources/queries/default/alter_table_drop_partition_3.sql
        • tajo-core/src/test/java/org/apache/tajo/client/TestTajoClient.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/AlreadyExistsAssumedPartitionDirectoryException.java
        • tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java
        • tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java
        • tajo-core/src/test/java/org/apache/tajo/cli/tsql/TestTajoCli.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java
        • tajo-core/src/test/resources/queries/TestAlterTable/alter_table_drop_partition1.sql
        • tajo-core/src/test/resources/results/TestTajoCli/testAlterTableAddPartition.result
        • tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java
        • tajo-docs/src/main/sphinx/sql_language/alter_table.rst
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionException.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #750 (See https://builds.apache.org/job/Tajo-master-build/750/ ) TAJO-1345 : Implement logical plan part and DDL executor for alter partition. (jaehwa) (blrunner: rev 3dba8c7e6bc539b07b136b959aa91dc4402be380) tajo-core/src/test/resources/queries/TestAlterTable/alter_table_add_partition1.sql tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/partition/PartitionKey.java tajo-catalog/tajo-catalog-drivers/tajo-hive/src/test/java/org/apache/tajo/catalog/store/TestHiveCatalogStore.java tajo-core/src/test/java/org/apache/tajo/engine/planner/TestLogicalPlanner.java tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java tajo-docs/src/main/sphinx/sql_language.rst tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/partition/PartitionDesc.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/AlreadyExistsPartitionException.java tajo-core/src/test/resources/queries/TestAlterTable/create_partitioned_table.sql tajo-plan/src/main/java/org/apache/tajo/plan/verifier/PreLogicalPlanVerifier.java tajo-core/src/test/resources/results/TestTajoCli/testAlterTableDropPartition.result tajo-core/src/test/java/org/apache/tajo/jdbc/TestTajoJdbc.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoPartitionedTableException.java tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeSerializer.java tajo-core/src/test/resources/results/TestTajoCli/testAlterTableAddDropPartition.result tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeDeserializer.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java CHANGES tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java tajo-plan/src/main/proto/Plan.proto tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 tajo-core/src/test/resources/queries/default/alter_table_drop_partition_3.sql tajo-core/src/test/java/org/apache/tajo/client/TestTajoClient.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/AlreadyExistsAssumedPartitionDirectoryException.java tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java tajo-core/src/test/java/org/apache/tajo/cli/tsql/TestTajoCli.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java tajo-core/src/test/resources/queries/TestAlterTable/alter_table_drop_partition1.sql tajo-core/src/test/resources/results/TestTajoCli/testAlterTableAddPartition.result tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java tajo-docs/src/main/sphinx/sql_language/alter_table.rst tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionException.java
        Hide
        hudson Hudson added a comment -

        ABORTED: Integrated in Tajo-master-CODEGEN-build #390 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/390/)
        TAJO-1345: Implement logical plan part and DDL executor for alter partition. (jaehwa) (blrunner: rev 3dba8c7e6bc539b07b136b959aa91dc4402be380)

        • tajo-core/src/test/resources/results/TestTajoCli/testAlterTableDropPartition.result
        • tajo-core/src/test/java/org/apache/tajo/jdbc/TestTajoJdbc.java
        • tajo-core/src/test/resources/queries/TestAlterTable/alter_table_drop_partition1.sql
        • tajo-plan/src/main/proto/Plan.proto
        • tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeSerializer.java
        • tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/AlreadyExistsPartitionException.java
        • tajo-core/src/test/resources/queries/TestAlterTable/create_partitioned_table.sql
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java
        • tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java
        • tajo-core/src/test/resources/results/TestTajoCli/testAlterTableAddPartition.result
        • tajo-catalog/tajo-catalog-drivers/tajo-hive/src/test/java/org/apache/tajo/catalog/store/TestHiveCatalogStore.java
        • tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java
        • tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java
        • tajo-docs/src/main/sphinx/sql_language/alter_table.rst
        • tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java
        • tajo-docs/src/main/sphinx/sql_language.rst
        • tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/AlreadyExistsAssumedPartitionDirectoryException.java
        • tajo-core/src/test/java/org/apache/tajo/engine/planner/TestLogicalPlanner.java
        • tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java
        • tajo-core/src/test/java/org/apache/tajo/cli/tsql/TestTajoCli.java
        • tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/partition/PartitionKey.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoPartitionedTableException.java
        • tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4
        • tajo-core/src/test/resources/results/TestTajoCli/testAlterTableAddDropPartition.result
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/partition/PartitionDesc.java
        • tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java
        • tajo-core/src/test/resources/queries/TestAlterTable/alter_table_add_partition1.sql
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionException.java
        • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java
        • tajo-core/src/test/resources/queries/default/alter_table_drop_partition_3.sql
        • CHANGES
        • tajo-core/src/test/java/org/apache/tajo/client/TestTajoClient.java
        • tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeDeserializer.java
        • tajo-plan/src/main/java/org/apache/tajo/plan/verifier/PreLogicalPlanVerifier.java
        Show
        hudson Hudson added a comment - ABORTED: Integrated in Tajo-master-CODEGEN-build #390 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/390/ ) TAJO-1345 : Implement logical plan part and DDL executor for alter partition. (jaehwa) (blrunner: rev 3dba8c7e6bc539b07b136b959aa91dc4402be380) tajo-core/src/test/resources/results/TestTajoCli/testAlterTableDropPartition.result tajo-core/src/test/java/org/apache/tajo/jdbc/TestTajoJdbc.java tajo-core/src/test/resources/queries/TestAlterTable/alter_table_drop_partition1.sql tajo-plan/src/main/proto/Plan.proto tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeSerializer.java tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/AlreadyExistsPartitionException.java tajo-core/src/test/resources/queries/TestAlterTable/create_partitioned_table.sql tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionKeyException.java tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java tajo-core/src/test/resources/results/TestTajoCli/testAlterTableAddPartition.result tajo-catalog/tajo-catalog-drivers/tajo-hive/src/test/java/org/apache/tajo/catalog/store/TestHiveCatalogStore.java tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java tajo-docs/src/main/sphinx/sql_language/alter_table.rst tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java tajo-docs/src/main/sphinx/sql_language.rst tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/AlreadyExistsAssumedPartitionDirectoryException.java tajo-core/src/test/java/org/apache/tajo/engine/planner/TestLogicalPlanner.java tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java tajo-core/src/test/java/org/apache/tajo/cli/tsql/TestTajoCli.java tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/partition/PartitionKey.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoPartitionedTableException.java tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 tajo-core/src/test/resources/results/TestTajoCli/testAlterTableAddDropPartition.result tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/partition/PartitionDesc.java tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java tajo-core/src/test/resources/queries/TestAlterTable/alter_table_add_partition1.sql tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/exception/NoSuchPartitionException.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java tajo-core/src/test/resources/queries/default/alter_table_drop_partition_3.sql CHANGES tajo-core/src/test/java/org/apache/tajo/client/TestTajoClient.java tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeDeserializer.java tajo-plan/src/main/java/org/apache/tajo/plan/verifier/PreLogicalPlanVerifier.java

          People

          • Assignee:
            blrunner Jaehwa Jung
            Reporter:
            blrunner Jaehwa Jung
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development