Details

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

      Description

      For simple demonstration of the intention. I'll apply to all execs when TAJO-1460 is included in master branch.

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user navis opened a pull request:

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

        TAJO-1484 Apply on ColPartitionStoreExec

        Based on TAJO-1460

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

        $ git pull https://github.com/navis/tajo TAJO-1484

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

        https://github.com/apache/tajo/pull/485.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 #485


        commit 78379a3ed39d5076b50434b1fef7bd67ddeb99d2
        Author: navis.ryu <navis@apache.org>
        Date: 2015-03-27T13:34:23Z

        TAJO-1460 Apply TAJO-1407 to ExternalSortExec

        commit 3279ad30380f68df6087b9cd818f659b77ba109a
        Author: navis.ryu <navis@apache.org>
        Date: 2015-03-30T09:24:31Z

        TAJO-1484 Apply on ColPartitionStoreExec


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user navis opened a pull request: https://github.com/apache/tajo/pull/485 TAJO-1484 Apply on ColPartitionStoreExec Based on TAJO-1460 You can merge this pull request into a Git repository by running: $ git pull https://github.com/navis/tajo TAJO-1484 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/485.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 #485 commit 78379a3ed39d5076b50434b1fef7bd67ddeb99d2 Author: navis.ryu <navis@apache.org> Date: 2015-03-27T13:34:23Z TAJO-1460 Apply TAJO-1407 to ExternalSortExec commit 3279ad30380f68df6087b9cd818f659b77ba109a Author: navis.ryu <navis@apache.org> Date: 2015-03-30T09:24:31Z TAJO-1484 Apply on ColPartitionStoreExec
        Hide
        navis Navis added a comment -

        Cannot reproduce VM crash in local.

        Show
        navis Navis added a comment - Cannot reproduce VM crash in local.
        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/12709377/TAJO-1484.1.patch.txt
        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 doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +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-core.

        Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/699//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/699//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html
        Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/699//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/12709377/TAJO-1484.1.patch.txt 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 doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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-core. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/699//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/699//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-core.html Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/699//console This message is automatically generated.
        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/485#discussion_r29142927

        — Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SortBasedColPartitionStoreExec.java —
        @@ -36,35 +36,24 @@

        • ascending or descending order of partition columns.
          */
          public class SortBasedColPartitionStoreExec extends ColPartitionStoreExec {
        • private Tuple currentKey;
        • private Tuple prevKey;
          +
          + private ComparableTuple prevKey;

        public SortBasedColPartitionStoreExec(TaskAttemptContext context, StoreTableNode plan, PhysicalExec child)
        throws IOException

        { super(context, plan, child); }
        • public void init() throws IOException { - super.init(); - - currentKey = new VTuple(keyNum); - }

          -

        • private void fillKeyTuple(Tuple inTuple, Tuple keyTuple) {
        • for (int i = 0; i < keyIds.length; i++) { - keyTuple.put(i, inTuple.get(keyIds[i])); - }
        • }
          + private transient StringBuilder sb = new StringBuilder();

        private String getSubdirectory(Tuple keyTuple) {
        — End diff –

        Input tuple seems not to be a key tuple anymore. Please change its name, too.

        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/485#discussion_r29142927 — Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SortBasedColPartitionStoreExec.java — @@ -36,35 +36,24 @@ ascending or descending order of partition columns. */ public class SortBasedColPartitionStoreExec extends ColPartitionStoreExec { private Tuple currentKey; private Tuple prevKey; + + private ComparableTuple prevKey; public SortBasedColPartitionStoreExec(TaskAttemptContext context, StoreTableNode plan, PhysicalExec child) throws IOException { super(context, plan, child); } public void init() throws IOException { - super.init(); - - currentKey = new VTuple(keyNum); - } - private void fillKeyTuple(Tuple inTuple, Tuple keyTuple) { for (int i = 0; i < keyIds.length; i++) { - keyTuple.put(i, inTuple.get(keyIds[i])); - } } + private transient StringBuilder sb = new StringBuilder(); private String getSubdirectory(Tuple keyTuple) { — End diff – Input tuple seems not to be a key tuple anymore. Please change its name, too.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user jihoonson commented on the pull request:

        https://github.com/apache/tajo/pull/485#issuecomment-96644896

        Hi @navis. Your patch looks good to me. I left only a single trivial comment.
        Additionally, I have a question about ```TupleType``` even though this is not related to this patch. Obviously, ```TupleType``` represents the data type rather than tuple type (even I have no idea what tuple type means). I wonder why you don't use just ```TajoDataTypes.Type```.

        Finally, truly sorry for late review, but would you rebase your patches? We have resolved the problem of jvm crash at https://issues.apache.org/jira/browse/TAJO-1568. If you trigger CI by rebasing your patches, it is definitely helpful for review.

        Show
        githubbot ASF GitHub Bot added a comment - Github user jihoonson commented on the pull request: https://github.com/apache/tajo/pull/485#issuecomment-96644896 Hi @navis. Your patch looks good to me. I left only a single trivial comment. Additionally, I have a question about ```TupleType``` even though this is not related to this patch. Obviously, ```TupleType``` represents the data type rather than tuple type (even I have no idea what tuple type means). I wonder why you don't use just ```TajoDataTypes.Type```. Finally, truly sorry for late review, but would you rebase your patches? We have resolved the problem of jvm crash at https://issues.apache.org/jira/browse/TAJO-1568 . If you trigger CI by rebasing your patches, it is definitely helpful for review.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on the pull request:

        https://github.com/apache/tajo/pull/485#issuecomment-124263688

        +1
        The patch looks good to me. I rebased the patch against the latest patch. It was trivial. I manually tested this patch. All unit tests are passed.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/485#issuecomment-124263688 +1 The patch looks good to me. I rebased the patch against the latest patch. It was trivial. I manually tested this patch. All unit tests are passed.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tajo/pull/485
        Hide
        hyunsik Hyunsik Choi added a comment -

        I just committed the patch to master branch. Thank you for your contribution.

        Show
        hyunsik Hyunsik Choi added a comment - I just committed the patch to master branch. Thank you for your contribution.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Tajo-master-build #768 (See https://builds.apache.org/job/Tajo-master-build/768/)
        TAJO-1484 Apply on ColPartitionStoreExec. (Contributed by Navis, committed by hyunsik) (hyunsik: rev c2aef7d367859b3c6a08ef82c5024bd73bb5d62b)

        • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SortBasedColPartitionStoreExec.java
        • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/ColPartitionStoreExec.java
        • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashBasedColPartitionStoreExec.java
        • CHANGES
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Tajo-master-build #768 (See https://builds.apache.org/job/Tajo-master-build/768/ ) TAJO-1484 Apply on ColPartitionStoreExec. (Contributed by Navis, committed by hyunsik) (hyunsik: rev c2aef7d367859b3c6a08ef82c5024bd73bb5d62b) tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SortBasedColPartitionStoreExec.java tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/ColPartitionStoreExec.java tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashBasedColPartitionStoreExec.java CHANGES
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Tajo-master-CODEGEN-build #406 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/406/)
        TAJO-1484 Apply on ColPartitionStoreExec. (Contributed by Navis, committed by hyunsik) (hyunsik: rev c2aef7d367859b3c6a08ef82c5024bd73bb5d62b)

        • CHANGES
        • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/ColPartitionStoreExec.java
        • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashBasedColPartitionStoreExec.java
        • tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SortBasedColPartitionStoreExec.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Tajo-master-CODEGEN-build #406 (See https://builds.apache.org/job/Tajo-master-CODEGEN-build/406/ ) TAJO-1484 Apply on ColPartitionStoreExec. (Contributed by Navis, committed by hyunsik) (hyunsik: rev c2aef7d367859b3c6a08ef82c5024bd73bb5d62b) CHANGES tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/ColPartitionStoreExec.java tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashBasedColPartitionStoreExec.java tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SortBasedColPartitionStoreExec.java

          People

          • Assignee:
            navis Navis
            Reporter:
            navis Navis
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development