Uploaded image for project: 'Tajo'
  1. Tajo
  2. TAJO-1421

Add 'ALTER TABLE SET PROPERTY' statement

    Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.11.0
    • Component/s: Catalog, Storage
    • Labels:
      None

      Description

      Each table has table properties. Currently, users can only set table properties only when a table is created. But, in many cases, we need to change table properties later.

      To allow users to change table properties of an existing table, I propose 'ALTER TABLE SET PROPERTY' statement.

      The proposed grammar is as follow:

      ALTER TABLE SET PROPERTY property_name = property_value, ..., property_name = property_value;
      

      property_name=property_value can be listed by comma chain.

      1. TAJO-1421_2.patch
        31 kB
        Yongjin Choi
      2. TAJO-1421_3.patch
        37 kB
        Yongjin Choi
      3. TAJO-1421_4.patch
        34 kB
        Yongjin Choi
      4. TAJO-1421.yongjin.patch
        31 kB
        Yongjin Choi

        Issue Links

          Activity

          Hide
          yongjin.choi Yongjin Choi added a comment -

          If you don't mind, may I start working on this issue?
          This feature would be useful in many tests and real applications.

          Show
          yongjin.choi Yongjin Choi added a comment - If you don't mind, may I start working on this issue? This feature would be useful in many tests and real applications.
          Hide
          tajoqa Tajo QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12706469/TAJO-1421.yongjin.patch
          against master revision release-0.9.0-rc0-214-g8d0146b.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The applied patch does not increase the total number of javadoc warnings.

          +1 checkstyle. The patch generated 0 code style errors.

          -1 findbugs. The patch appears to introduce 3 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in tajo-algebra tajo-catalog/tajo-catalog-common tajo-catalog/tajo-catalog-drivers/tajo-hcatalog tajo-catalog/tajo-catalog-server tajo-core tajo-plan.

          Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/643//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/643//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/643//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-catalog-server.html
          Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/643//console

          This message is automatically generated.

          Show
          tajoqa Tajo QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12706469/TAJO-1421.yongjin.patch against master revision release-0.9.0-rc0-214-g8d0146b. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. -1 findbugs. The patch appears to introduce 3 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in tajo-algebra tajo-catalog/tajo-catalog-common tajo-catalog/tajo-catalog-drivers/tajo-hcatalog tajo-catalog/tajo-catalog-server tajo-core tajo-plan. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/643//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/643//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/643//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-catalog-server.html Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/643//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blrunner commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-87507492

          Thanks for your contribution.
          I'll review today.

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-87507492 Thanks for your contribution. I'll review today.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blrunner commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-87584764

          @superpiggy

          Could you rebase this? I failed to merge this as following:

          ```
          git pull https://github.com/superpiggy/tajo TAJO-1421
          remote: Counting objects: 235, done.
          remote: Compressing objects: 100% (50/50), done.
          remote: Total 235 (delta 31), reused 19 (delta 19), pack-reused 166
          Receiving objects: 100% (235/235), 133.68 KiB | 78.00 KiB/s, done.
          Resolving deltas: 100% (38/38), done.
          From https://github.com/superpiggy/tajo

          • branch TAJO-1421 -> FETCH_HEAD
            Auto-merging tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java
            Auto-merging tajo-core/src/test/java/org/apache/tajo/QueryTestCaseBase.java
            Auto-merging tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java
            Auto-merging tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4
            Auto-merging tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4
            Auto-merging tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java
            Auto-merging tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java
            CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java
            Auto-merging tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java
            CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java
            Auto-merging tajo-catalog/tajo-catalog-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java
            CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java
            Auto-merging tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto
            CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto
            Auto-merging tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java
            CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java
            Auto-merging tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java
            CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java
            Automatic merge failed; fix conflicts and then commit the result.
            ```
          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-87584764 @superpiggy Could you rebase this? I failed to merge this as following: ``` git pull https://github.com/superpiggy/tajo TAJO-1421 remote: Counting objects: 235, done. remote: Compressing objects: 100% (50/50), done. remote: Total 235 (delta 31), reused 19 (delta 19), pack-reused 166 Receiving objects: 100% (235/235), 133.68 KiB | 78.00 KiB/s, done. Resolving deltas: 100% (38/38), done. From https://github.com/superpiggy/tajo branch TAJO-1421 -> FETCH_HEAD Auto-merging tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java Auto-merging tajo-core/src/test/java/org/apache/tajo/QueryTestCaseBase.java Auto-merging tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java Auto-merging tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 Auto-merging tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4 Auto-merging tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java Auto-merging tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java Auto-merging tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java Auto-merging tajo-catalog/tajo-catalog-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java Auto-merging tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto Auto-merging tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java Auto-merging tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java CONFLICT (content): Merge conflict in tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java Automatic merge failed; fix conflicts and then commit the result. ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user superpiggy commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-87973053

          @blrunner
          Thank you for your interest.
          I merged recent master to TAJO-1421 branch.

          Show
          githubbot ASF GitHub Bot added a comment - Github user superpiggy commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-87973053 @blrunner Thank you for your interest. I merged recent master to TAJO-1421 branch.
          Hide
          tajoqa Tajo QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12708357/TAJO-1421_2.patch
          against master revision release-0.9.0-rc0-227-g487a0e5.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 11 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The applied patch does not increase the total number of javadoc warnings.

          +1 checkstyle. The patch generated 0 code style errors.

          -1 findbugs. The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in tajo-algebra tajo-catalog/tajo-catalog-common tajo-catalog/tajo-catalog-drivers/tajo-hcatalog tajo-catalog/tajo-catalog-server tajo-core tajo-plan.

          Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/684//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/684//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-catalog-server.html
          Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/684//console

          This message is automatically generated.

          Show
          tajoqa Tajo QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12708357/TAJO-1421_2.patch against master revision release-0.9.0-rc0-227-g487a0e5. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 11 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. -1 findbugs. The patch appears to introduce 2 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in tajo-algebra tajo-catalog/tajo-catalog-common tajo-catalog/tajo-catalog-drivers/tajo-hcatalog tajo-catalog/tajo-catalog-server tajo-core tajo-plan. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/684//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/684//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-catalog-server.html Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/684//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/450#discussion_r27637718

          — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java —
          @@ -44,10 +45,16 @@
          protected Column addColumn = null; //optional
          @Expose
          protected PartitionDesc partitionDesc; //optional
          + @Expose
          + protected KeyValueSet properties;

          public AlterTableDesc()

          { + this.properties = new KeyValueSet(); }

          + public AlterTableDesc(KeyValueSet properties) {
          — End diff –

          It looks like an unnecessary constructor because it is not used.

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on a diff in the pull request: https://github.com/apache/tajo/pull/450#discussion_r27637718 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java — @@ -44,10 +45,16 @@ protected Column addColumn = null; //optional @Expose protected PartitionDesc partitionDesc; //optional + @Expose + protected KeyValueSet properties; public AlterTableDesc() { + this.properties = new KeyValueSet(); } + public AlterTableDesc(KeyValueSet properties) { — End diff – It looks like an unnecessary constructor because it is not used.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/450#discussion_r27637762

          — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java —
          @@ -44,10 +45,16 @@
          protected Column addColumn = null; //optional
          @Expose
          protected PartitionDesc partitionDesc; //optional
          + @Expose
          + protected KeyValueSet properties;

          public AlterTableDesc() {
          + this.properties = new KeyValueSet();
          — End diff –

          Could you separate this code to another method such as AlterTableDesc::init?

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on a diff in the pull request: https://github.com/apache/tajo/pull/450#discussion_r27637762 — Diff: tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java — @@ -44,10 +45,16 @@ protected Column addColumn = null; //optional @Expose protected PartitionDesc partitionDesc; //optional + @Expose + protected KeyValueSet properties; public AlterTableDesc() { + this.properties = new KeyValueSet(); — End diff – Could you separate this code to another method such as AlterTableDesc::init?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/450#discussion_r27710739

          — Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java —
          @@ -1023,6 +1026,77 @@ public void alterTable(CatalogProtos.AlterTableDescProto alterTableDescProto) th

          }

          + private Map<String, String> getTableOptions(final int tableId) throws CatalogException {
          + Connection conn = null;
          + PreparedStatement pstmt = null;
          + ResultSet res = null;
          + Map<String, String> options = TUtil.newHashMap();
          +
          + try {
          + String tidSql = "SELECT key_, value_ FROM " + TB_OPTIONS + " WHERE TID=?";
          +
          + conn = getConnection();
          + pstmt = conn.prepareStatement(tidSql);
          + pstmt.setInt(1, tableId);
          + res = pstmt.executeQuery();
          +
          + while (res.next()) {
          + String key = res.getString("KEY_");
          + String value = res.getString("VALUE_");
          + options.put(key, value);
          — End diff –

          New objects looks like unnecessary resources. I would like t more better if you don't create new String objects as follows.
          ```
          options.put(res.getString("KEY_"), res.getString("VALUE_"));
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on a diff in the pull request: https://github.com/apache/tajo/pull/450#discussion_r27710739 — Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java — @@ -1023,6 +1026,77 @@ public void alterTable(CatalogProtos.AlterTableDescProto alterTableDescProto) th } + private Map<String, String> getTableOptions(final int tableId) throws CatalogException { + Connection conn = null; + PreparedStatement pstmt = null; + ResultSet res = null; + Map<String, String> options = TUtil.newHashMap(); + + try { + String tidSql = "SELECT key_, value_ FROM " + TB_OPTIONS + " WHERE TID=?"; + + conn = getConnection(); + pstmt = conn.prepareStatement(tidSql); + pstmt.setInt(1, tableId); + res = pstmt.executeQuery(); + + while (res.next()) { + String key = res.getString("KEY_"); + String value = res.getString("VALUE_"); + options.put(key, value); — End diff – New objects looks like unnecessary resources. I would like t more better if you don't create new String objects as follows. ``` options.put(res.getString("KEY_"), res.getString("VALUE_")); ```
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/450#discussion_r27711220

          — Diff: tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java —
          @@ -995,6 +995,10 @@ public void testAlterTableName () throws Exception {
          TableDesc addColumnDesc = catalog.getTableDesc("default","mynewcooltable");
          assertTrue(addColumnDesc.getSchema().containsByName("mynewcol"));

          + //SET_PROPERTY
          + catalog.alterTable(createMockAlterTableSetProperty());
          + TableDesc setPropertyDesc = catalog.getTableDesc("default","mynewcooltable");
          + assertEquals("GMT+0", setPropertyDesc.getMeta().getOption("timezone"));
          — End diff –

          You already implemented an unit test case for this feature. But I suggest for you to implement more unit test cases because you just used mockup table. It would be more valuable if you verify the change status of TableDesc. Unit test cases for partitioned table would be help for you to implement this.

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on a diff in the pull request: https://github.com/apache/tajo/pull/450#discussion_r27711220 — Diff: tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java — @@ -995,6 +995,10 @@ public void testAlterTableName () throws Exception { TableDesc addColumnDesc = catalog.getTableDesc("default","mynewcooltable"); assertTrue(addColumnDesc.getSchema().containsByName("mynewcol")); + //SET_PROPERTY + catalog.alterTable(createMockAlterTableSetProperty()); + TableDesc setPropertyDesc = catalog.getTableDesc("default","mynewcooltable"); + assertEquals("GMT+0", setPropertyDesc.getMeta().getOption("timezone")); — End diff – You already implemented an unit test case for this feature. But I suggest for you to implement more unit test cases because you just used mockup table. It would be more valuable if you verify the change status of TableDesc. Unit test cases for partitioned table would be help for you to implement this.
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/450#discussion_r27711297

          — Diff: tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java —
          @@ -485,6 +485,18 @@ public void testAlterTableDropPartition3() throws IOException {
          }

          @Test
          + public void testAlterTableSetProperty1() throws IOException {
          — End diff –

          If possible, could you add more unit test cases such as, text delimiter, compression codec, etc?

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on a diff in the pull request: https://github.com/apache/tajo/pull/450#discussion_r27711297 — Diff: tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java — @@ -485,6 +485,18 @@ public void testAlterTableDropPartition3() throws IOException { } @Test + public void testAlterTableSetProperty1() throws IOException { — End diff – If possible, could you add more unit test cases such as, text delimiter, compression codec, etc?
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/tajo/pull/450#discussion_r27711320

          — Diff: tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java —
          @@ -49,4 +50,18 @@ public final void testAlterTableAddNewColumn() throws Exception

          { assertColumnExists(createdNames.get(0),"cool"); }

          + @Test
          + public final void testAlterTableSetProperty() throws Exception {
          — End diff –

          I would like it more better If you add more unit test cases such as, text delimiter, compression codec.

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on a diff in the pull request: https://github.com/apache/tajo/pull/450#discussion_r27711320 — Diff: tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java — @@ -49,4 +50,18 @@ public final void testAlterTableAddNewColumn() throws Exception { assertColumnExists(createdNames.get(0),"cool"); } + @Test + public final void testAlterTableSetProperty() throws Exception { — End diff – I would like it more better If you add more unit test cases such as, text delimiter, compression codec.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user superpiggy commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-89121918

          @blrunner
          Thank you so much for your kind review.
          I will update the patch soon following your advice.

          Show
          githubbot ASF GitHub Bot added a comment - Github user superpiggy commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-89121918 @blrunner Thank you so much for your kind review. I will update the patch soon following your advice.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user superpiggy commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-89228080

          @blrunner
          I added more tests and removed unnecessary codes according to your comments.

          Show
          githubbot ASF GitHub Bot added a comment - Github user superpiggy commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-89228080 @blrunner I added more tests and removed unnecessary codes according to your comments.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blrunner commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-89240628

          It looks good to me. Before I give +1 to this, I want to assure CI build result. Could you trigger the travis CI build and upload the latest patch file to JIRA?

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-89240628 It looks good to me. Before I give +1 to this, I want to assure CI build result. Could you trigger the travis CI build and upload the latest patch file to JIRA?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user superpiggy commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-89248812

          I uploaded the patch to tajo JIRA.
          Thanks @blrunner

          Show
          githubbot ASF GitHub Bot added a comment - Github user superpiggy commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-89248812 I uploaded the patch to tajo JIRA. Thanks @blrunner
          Hide
          tajoqa Tajo QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12709209/TAJO-1421_3.patch
          against master revision release-0.9.0-rc0-236-g70d5fdf.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 19 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The applied patch does not increase the total number of javadoc warnings.

          +1 checkstyle. The patch generated 0 code style errors.

          -1 findbugs. The patch appears to introduce 5 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in tajo-algebra tajo-catalog/tajo-catalog-common tajo-catalog/tajo-catalog-drivers/tajo-hcatalog tajo-catalog/tajo-catalog-server tajo-core tajo-plan:
          org.apache.tajo.engine.query.TestAlterTable

          Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/698//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/698//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/698//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-catalog-server.html
          Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/698//console

          This message is automatically generated.

          Show
          tajoqa Tajo QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12709209/TAJO-1421_3.patch against master revision release-0.9.0-rc0-236-g70d5fdf. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 19 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. -1 findbugs. The patch appears to introduce 5 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in tajo-algebra tajo-catalog/tajo-catalog-common tajo-catalog/tajo-catalog-drivers/tajo-hcatalog tajo-catalog/tajo-catalog-server tajo-core tajo-plan: org.apache.tajo.engine.query.TestAlterTable Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/698//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/698//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/698//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-catalog-server.html Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/698//console This message is automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blrunner commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-89669989

          @superpiggy

          It seems that there is a timezone difference between your environment and build server environment. I suggest for you to replace timezone unit test to another unit test.

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-89669989 @superpiggy It seems that there is a timezone difference between your environment and build server environment. I suggest for you to replace timezone unit test to another unit test.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user superpiggy commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-89737523

          @blrunner

          You're right.
          If you don't mind, may I remove timezone query test case?
          I already added another query test using 'text.delimiter'.
          And there is no difference in code flow for 'alter table set property'.

          Show
          githubbot ASF GitHub Bot added a comment - Github user superpiggy commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-89737523 @blrunner You're right. If you don't mind, may I remove timezone query test case? I already added another query test using 'text.delimiter'. And there is no difference in code flow for 'alter table set property'.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user blrunner commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-89741591

          Sure, go ahead.

          Show
          githubbot ASF GitHub Bot added a comment - Github user blrunner commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-89741591 Sure, go ahead.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user superpiggy commented on the pull request:

          https://github.com/apache/tajo/pull/450#issuecomment-89756504

          @blrunner
          Thank you, I will upload the final patch in a minute.

          Show
          githubbot ASF GitHub Bot added a comment - Github user superpiggy commented on the pull request: https://github.com/apache/tajo/pull/450#issuecomment-89756504 @blrunner Thank you, I will upload the final patch in a minute.
          Hide
          tajoqa Tajo QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12709467/TAJO-1421_4.patch
          against master revision release-0.9.0-rc0-237-gb0abff8.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 15 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The applied patch does not increase the total number of javadoc warnings.

          +1 checkstyle. The patch generated 0 code style errors.

          -1 findbugs. The patch appears to introduce 5 new Findbugs (version 2.0.3) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in tajo-algebra tajo-catalog/tajo-catalog-common tajo-catalog/tajo-catalog-drivers/tajo-hcatalog tajo-catalog/tajo-catalog-server tajo-core tajo-plan.

          Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/707//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/707//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/707//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-catalog-server.html
          Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/707//console

          This message is automatically generated.

          Show
          tajoqa Tajo QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12709467/TAJO-1421_4.patch against master revision release-0.9.0-rc0-237-gb0abff8. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The applied patch does not increase the total number of javadoc warnings. +1 checkstyle. The patch generated 0 code style errors. -1 findbugs. The patch appears to introduce 5 new Findbugs (version 2.0.3) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in tajo-algebra tajo-catalog/tajo-catalog-common tajo-catalog/tajo-catalog-drivers/tajo-hcatalog tajo-catalog/tajo-catalog-server tajo-core tajo-plan. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/707//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/707//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/707//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-catalog-server.html Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/707//console This message is automatically generated.
          Hide
          blrunner Jaehwa Jung added a comment -

          +1

          Thanks Yongjin Choi
          I'll ship it soon.

          Show
          blrunner Jaehwa Jung added a comment - +1 Thanks Yongjin Choi I'll ship it soon.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

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

          I've just committed this to the master branch.

          Show
          blrunner Jaehwa Jung added a comment - I've just committed this to the master branch.
          Hide
          hudson Hudson added a comment -

          FAILURE: Integrated in Tajo-master-CODEGEN-build #289 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/289/)
          TAJO-1421: Add 'ALTER TABLE SET PROPERTY' statement. (blrunner: rev dd43373418b1857a1f694a08c06da77a86d3cfeb)

          • tajo-core/src/test/resources/queries/TestAlterTable/table2_ddl.sql
          • tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java
          • tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java
          • tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java
          • tajo-catalog/tajo-catalog-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java
          • tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java
          • tajo-core/src/test/resources/queries/default/alter_table_set_property_3.sql
          • tajo-plan/src/main/proto/Plan.proto
          • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java
          • tajo-core/src/test/resources/queries/TestAlterTable/testAlterTableSetProperty2.sql
          • tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto
          • CHANGES
          • tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeDeserializer.java
          • tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java
          • tajo-core/src/test/resources/queries/default/alter_table_set_property_2.sql
          • tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeSerializer.java
          • tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4
          • tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java
          • tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java
          • tajo-core/src/test/resources/dataset/TestAlterTable/table2.tbl
          • tajo-core/src/test/resources/results/TestAlterTable/after_set_property_delimiter.result
          • tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java
          • tajo-core/src/test/resources/queries/TestAlterTable/testAlterTableSetProperty.sql
          • tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java
          • tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4
          • tajo-core/src/test/java/org/apache/tajo/QueryTestCaseBase.java
          • tajo-core/src/test/resources/results/TestAlterTable/before_set_property_delimiter.result
          • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java
          • tajo-core/src/test/resources/queries/TestAlterTable/alter_table_set_property_timezone.sql
          • tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTableOpType.java
          • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java
          • tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java
          • tajo-core/src/test/resources/queries/TestAlterTable/alter_table_set_property_delimiter.sql
          • tajo-core/src/test/resources/queries/default/alter_table_set_property_1.sql
          Show
          hudson Hudson added a comment - FAILURE: Integrated in Tajo-master-CODEGEN-build #289 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/289/ ) TAJO-1421 : Add 'ALTER TABLE SET PROPERTY' statement. (blrunner: rev dd43373418b1857a1f694a08c06da77a86d3cfeb) tajo-core/src/test/resources/queries/TestAlterTable/table2_ddl.sql tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java tajo-catalog/tajo-catalog-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java tajo-core/src/test/resources/queries/default/alter_table_set_property_3.sql tajo-plan/src/main/proto/Plan.proto tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java tajo-core/src/test/resources/queries/TestAlterTable/testAlterTableSetProperty2.sql tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto CHANGES tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeDeserializer.java tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java tajo-core/src/test/resources/queries/default/alter_table_set_property_2.sql tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeSerializer.java tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4 tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java tajo-core/src/test/resources/dataset/TestAlterTable/table2.tbl tajo-core/src/test/resources/results/TestAlterTable/after_set_property_delimiter.result tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java tajo-core/src/test/resources/queries/TestAlterTable/testAlterTableSetProperty.sql tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 tajo-core/src/test/java/org/apache/tajo/QueryTestCaseBase.java tajo-core/src/test/resources/results/TestAlterTable/before_set_property_delimiter.result tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java tajo-core/src/test/resources/queries/TestAlterTable/alter_table_set_property_timezone.sql tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTableOpType.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java tajo-core/src/test/resources/queries/TestAlterTable/alter_table_set_property_delimiter.sql tajo-core/src/test/resources/queries/default/alter_table_set_property_1.sql
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Tajo-master-build #651 (See https://builds.apache.org/job/Tajo-master-build/651/)
          TAJO-1421: Add 'ALTER TABLE SET PROPERTY' statement. (blrunner: rev dd43373418b1857a1f694a08c06da77a86d3cfeb)

          • tajo-core/src/test/resources/results/TestAlterTable/before_set_property_delimiter.result
          • tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java
          • tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTableOpType.java
          • tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java
          • tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java
          • tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java
          • tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4
          • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java
          • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java
          • tajo-core/src/test/resources/queries/default/alter_table_set_property_3.sql
          • tajo-core/src/test/resources/queries/default/alter_table_set_property_2.sql
          • tajo-plan/src/main/proto/Plan.proto
          • tajo-core/src/test/java/org/apache/tajo/QueryTestCaseBase.java
          • CHANGES
          • tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeDeserializer.java
          • tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java
          • tajo-core/src/test/resources/queries/default/alter_table_set_property_1.sql
          • tajo-core/src/test/resources/queries/TestAlterTable/testAlterTableSetProperty2.sql
          • tajo-core/src/test/resources/results/TestAlterTable/after_set_property_delimiter.result
          • tajo-core/src/test/resources/queries/TestAlterTable/alter_table_set_property_timezone.sql
          • tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java
          • tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto
          • tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeSerializer.java
          • tajo-core/src/test/resources/queries/TestAlterTable/alter_table_set_property_delimiter.sql
          • tajo-core/src/test/resources/queries/TestAlterTable/testAlterTableSetProperty.sql
          • tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java
          • tajo-core/src/test/resources/queries/TestAlterTable/table2_ddl.sql
          • tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4
          • tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java
          • tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java
          • tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java
          • tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java
          • tajo-core/src/test/resources/dataset/TestAlterTable/table2.tbl
          • tajo-catalog/tajo-catalog-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Tajo-master-build #651 (See https://builds.apache.org/job/Tajo-master-build/651/ ) TAJO-1421 : Add 'ALTER TABLE SET PROPERTY' statement. (blrunner: rev dd43373418b1857a1f694a08c06da77a86d3cfeb) tajo-core/src/test/resources/results/TestAlterTable/before_set_property_delimiter.result tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTableOpType.java tajo-core/src/main/java/org/apache/tajo/master/exec/DDLExecutor.java tajo-catalog/tajo-catalog-server/src/test/java/org/apache/tajo/catalog/TestCatalog.java tajo-core/src/test/java/org/apache/tajo/engine/parser/TestSQLAnalyzer.java tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLLexer.g4 tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableType.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/AlterTableDesc.java tajo-core/src/test/resources/queries/default/alter_table_set_property_3.sql tajo-core/src/test/resources/queries/default/alter_table_set_property_2.sql tajo-plan/src/main/proto/Plan.proto tajo-core/src/test/java/org/apache/tajo/QueryTestCaseBase.java CHANGES tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeDeserializer.java tajo-algebra/src/main/java/org/apache/tajo/algebra/AlterTable.java tajo-core/src/test/resources/queries/default/alter_table_set_property_1.sql tajo-core/src/test/resources/queries/TestAlterTable/testAlterTableSetProperty2.sql tajo-core/src/test/resources/results/TestAlterTable/after_set_property_delimiter.result tajo-core/src/test/resources/queries/TestAlterTable/alter_table_set_property_timezone.sql tajo-plan/src/main/java/org/apache/tajo/plan/logical/AlterTableNode.java tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto tajo-plan/src/main/java/org/apache/tajo/plan/serder/LogicalNodeSerializer.java tajo-core/src/test/resources/queries/TestAlterTable/alter_table_set_property_delimiter.sql tajo-core/src/test/resources/queries/TestAlterTable/testAlterTableSetProperty.sql tajo-core/src/main/java/org/apache/tajo/engine/parser/SQLAnalyzer.java tajo-core/src/test/resources/queries/TestAlterTable/table2_ddl.sql tajo-core/src/main/antlr4/org/apache/tajo/engine/parser/SQLParser.g4 tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java tajo-plan/src/main/java/org/apache/tajo/plan/LogicalPlanner.java tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java tajo-catalog/tajo-catalog-common/src/main/java/org/apache/tajo/catalog/CatalogUtil.java tajo-core/src/test/resources/dataset/TestAlterTable/table2.tbl tajo-catalog/tajo-catalog-drivers/tajo-hcatalog/src/main/java/org/apache/tajo/catalog/store/HCatalogStore.java

            People

            • Assignee:
              yongjin.choi Yongjin Choi
              Reporter:
              hyunsik Hyunsik Choi
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development