Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.10.0
    • Component/s: SQL Shell
    • Labels:
      None

      Description

      When multiple commands are inputted in a single line, tsql breaks them up into individual history entries and shows one by one when up arrow key is pressed.

      Change it so that all commands inputted together show as a group in the history as in bash or mysql

      1. TAJO-1159.Jung.111114.patch.txt
        1 kB
        Jaewoong Jung
      2. TAJO-1159.Jung.111214.patch.txt
        2 kB
        Jaewoong Jung
      3. TAJO-1159.Jung.111514.patch.txt
        3 kB
        Jaewoong Jung

        Activity

        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
        githubbot ASF GitHub Bot added a comment -

        Github user hyunsik commented on the pull request:

        https://github.com/apache/tajo/pull/316#issuecomment-68028359

        +1 patch looks good to me.

        Show
        githubbot ASF GitHub Bot added a comment - Github user hyunsik commented on the pull request: https://github.com/apache/tajo/pull/316#issuecomment-68028359 +1 patch looks good to me.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

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

        GitHub user gyias opened a pull request:

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

        TAJO-1159: Change tsql history behavior

        Sorry for being super-late. My laptop began to refuse running tajo, so I ended up setting a new machine for tajo development.

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

        $ git pull https://github.com/gyias/tajo tsql

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

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


        commit 70fef1aa1e8f347dcc955f86f86852f29e4a189f
        Author: Jaewoong Jung <gyias@users.noreply.github.com>
        Date: 2014-11-21T07:43:01Z

        Merge pull request #1 from apache/master

        Sync with original master

        commit 0137df1b04d7dcb37d2fe3ccacf62f035247565c
        Author: Jaewoong Jung <gyias@users.noreply.github.com>
        Date: 2014-12-02T07:56:39Z

        Merge pull request #2 from apache/master

        Merge latest changes

        commit 49c39b85b38983f99df708eb2a03b08c1deda7fe
        Author: Jaewoong Jung <jungjw@gmail.com>
        Date: 2014-12-12T07:52:06Z

        Merge remote-tracking branch 'upstream/master'

        commit 4cfb5b0a7c9e1c7737e89ce98ce9a2dedbde25aa
        Author: Jaewoong Jung <jungjw@gmail.com>
        Date: 2014-12-18T08:02:20Z

        Updates tsql history behavior.

        commit de99d1394263f12d3add3d196c526213f3f50a48
        Author: Jaewoong Jung <jungjw@gmail.com>
        Date: 2014-12-23T07:28:06Z

        Merge remote-tracking branch 'upstream/master'

        commit a5ffca1ac5ac0f989869bee563661a23dbe21c7a
        Author: Jaewoong Jung <jungjw@gmail.com>
        Date: 2014-12-23T07:30:05Z

        Merge branch 'master' into tsql


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user gyias opened a pull request: https://github.com/apache/tajo/pull/316 TAJO-1159 : Change tsql history behavior Sorry for being super-late. My laptop began to refuse running tajo, so I ended up setting a new machine for tajo development. You can merge this pull request into a Git repository by running: $ git pull https://github.com/gyias/tajo tsql Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tajo/pull/316.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 #316 commit 70fef1aa1e8f347dcc955f86f86852f29e4a189f Author: Jaewoong Jung <gyias@users.noreply.github.com> Date: 2014-11-21T07:43:01Z Merge pull request #1 from apache/master Sync with original master commit 0137df1b04d7dcb37d2fe3ccacf62f035247565c Author: Jaewoong Jung <gyias@users.noreply.github.com> Date: 2014-12-02T07:56:39Z Merge pull request #2 from apache/master Merge latest changes commit 49c39b85b38983f99df708eb2a03b08c1deda7fe Author: Jaewoong Jung <jungjw@gmail.com> Date: 2014-12-12T07:52:06Z Merge remote-tracking branch 'upstream/master' commit 4cfb5b0a7c9e1c7737e89ce98ce9a2dedbde25aa Author: Jaewoong Jung <jungjw@gmail.com> Date: 2014-12-18T08:02:20Z Updates tsql history behavior. commit de99d1394263f12d3add3d196c526213f3f50a48 Author: Jaewoong Jung <jungjw@gmail.com> Date: 2014-12-23T07:28:06Z Merge remote-tracking branch 'upstream/master' commit a5ffca1ac5ac0f989869bee563661a23dbe21c7a Author: Jaewoong Jung <jungjw@gmail.com> Date: 2014-12-23T07:30:05Z Merge branch 'master' into tsql
        Hide
        hyunsik Hyunsik Choi added a comment -

        Could you rebase the patch against latest revision?

        Show
        hyunsik Hyunsik Choi added a comment - Could you rebase the patch against latest revision?
        Hide
        hyunsik Hyunsik Choi added a comment -

        Hi Jaewoong Jung,

        I'm sorry for late review. I misunderstood that I already committed it. I'll finish the review by today.

        Show
        hyunsik Hyunsik Choi added a comment - Hi Jaewoong Jung , I'm sorry for late review. I misunderstood that I already committed it. I'll finish the review by today.
        Hide
        jungjw Jaewoong Jung added a comment -

        ping? care to take another look on this?

        Show
        jungjw Jaewoong Jung added a comment - ping? care to take another look on this?
        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/12681782/TAJO-1159.Jung.111514.patch.txt
        against master revision release-0.9.0-rc0-42-g6059a3d.

        +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 generated 358 release audit warnings.

        +1 core tests. The patch passed unit tests in tajo-client.

        Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/520//testReport/
        Release audit warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/520//artifact/incubator-tajo/patchprocess/patchReleaseAuditProblems.txt
        Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/520//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-client.html
        Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/520//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/12681782/TAJO-1159.Jung.111514.patch.txt against master revision release-0.9.0-rc0-42-g6059a3d. +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 generated 358 release audit warnings. +1 core tests. The patch passed unit tests in tajo-client. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/520//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/520//artifact/incubator-tajo/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/520//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-client.html Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/520//console This message is automatically generated.
        Hide
        jungjw Jaewoong Jung added a comment -

        That's a good point.

        I took a deeper look at mysql's behavior and found that if a statement spans multiple lines, it adds the complete statement to history in addition to individual lines when the statement is executed.

        For example, here's the most complex case I could think of and tried.

        mysql> select * from table1; select
        -> * from table2; select * from table3; select
        -> * from table4;

        Once you execute this, the mysql history contains the following entries

        (1) select * from table1; select
        (2) * from table2; select * from table3; select
        (3) select * from table2;
        (4) * from table4;
        (5) select * from table4;
        where (3) and (5) are the aforementioned additional complete statement entries.
        (Also note that "select * from table1" and "select * from table3" haven't been added separately since they don't span multiple lines.)

        I modified my change accordingly and confirmed that it works the same as mysql for all the cases I tried.

        Could you take another look?

        Thanks.

        Show
        jungjw Jaewoong Jung added a comment - That's a good point. I took a deeper look at mysql's behavior and found that if a statement spans multiple lines, it adds the complete statement to history in addition to individual lines when the statement is executed. For example, here's the most complex case I could think of and tried. mysql> select * from table1; select -> * from table2; select * from table3; select -> * from table4; Once you execute this, the mysql history contains the following entries (1) select * from table1; select (2) * from table2; select * from table3; select (3) select * from table2; (4) * from table4; (5) select * from table4; where (3) and (5) are the aforementioned additional complete statement entries. (Also note that "select * from table1" and "select * from table3" haven't been added separately since they don't span multiple lines.) I modified my change accordingly and confirmed that it works the same as mysql for all the cases I tried. Could you take another look? Thanks.
        Hide
        hyunsik Hyunsik Choi added a comment - - edited

        I missed one test in tsql. The test is about the history behavior of a SQL statement consisting of multiple lines. After this patch is applied, single SQL statement consisting of multiple lines are broken into multiple individual history lines in tsql history. You can reproduce the following example:

        $ bin/tsql
        
        default> select
        > x,
        > y
        > from table1;
        
        [when up arrow key is pressed]
        > from table1;
        

        When up arrow key is pressed, a single statement should be retrieved in the shell. Could you check this case?

        Show
        hyunsik Hyunsik Choi added a comment - - edited I missed one test in tsql. The test is about the history behavior of a SQL statement consisting of multiple lines. After this patch is applied, single SQL statement consisting of multiple lines are broken into multiple individual history lines in tsql history. You can reproduce the following example: $ bin/tsql default > select > x, > y > from table1; [when up arrow key is pressed] > from table1; When up arrow key is pressed, a single statement should be retrieved in the shell. Could you check this case?
        Hide
        hyunsik Hyunsik Choi added a comment -

        +1 for the latest patch.

        It fixes the problem to break of multiple meta commands in one line into multiple commands in history. The patch passes all unit tests, I tested the history behavior in tsql. It works correctly.

        Show
        hyunsik Hyunsik Choi added a comment - +1 for the latest patch. It fixes the problem to break of multiple meta commands in one line into multiple commands in history. The patch passes all unit tests, I tested the history behavior in tsql. It works correctly.
        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/12681008/TAJO-1159.Jung.111214.patch.txt
        against master revision release-0.9.0-rc0-39-g13af3d3.

        +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 generated 194 release audit warnings.

        +1 core tests. The patch passed unit tests in tajo-client.

        Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/519//testReport/
        Release audit warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/519//artifact/incubator-tajo/patchprocess/patchReleaseAuditProblems.txt
        Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/519//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-client.html
        Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/519//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/12681008/TAJO-1159.Jung.111214.patch.txt against master revision release-0.9.0-rc0-39-g13af3d3. +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 generated 194 release audit warnings. +1 core tests. The patch passed unit tests in tajo-client. Test results: https://builds.apache.org/job/PreCommit-TAJO-Build/519//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/519//artifact/incubator-tajo/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-TAJO-Build/519//artifact/incubator-tajo/patchprocess/newPatchFindbugsWarningstajo-client.html Console output: https://builds.apache.org/job/PreCommit-TAJO-Build/519//console This message is automatically generated.
        Hide
        jungjw Jaewoong Jung added a comment -

        Oops, something mush have gone wrong while trying to filter out irrelevant experimental changes. Here's my second try. Please take another look.

        BTW, I forgot to mention that JSON queries also show up in history with this change. I'm not sure why they have been excluded, but I personally found it very useful.

        Show
        jungjw Jaewoong Jung added a comment - Oops, something mush have gone wrong while trying to filter out irrelevant experimental changes. Here's my second try. Please take another look. BTW, I forgot to mention that JSON queries also show up in history with this change. I'm not sure why they have been excluded, but I personally found it very useful.
        Hide
        hyunsik Hyunsik Choi added a comment - - edited

        Hi Jaewoong Jung,

        Above all, thank you for your work. BTW, probably the patch seems to contain the difference only between last two revisions. Could you check the uploaded patch?

        You might change more things through multiple commits, but the submitted patch does not contain all changes. In your case, in order to get a collect patch, you should use the following commands:

        git fetch origin
        git diff --no-prefix origin/master > TAJO-1159.Jung.111114_2.patch.txt
        

        If you need more help, feel free to ask me anything.

        Show
        hyunsik Hyunsik Choi added a comment - - edited Hi Jaewoong Jung , Above all, thank you for your work. BTW, probably the patch seems to contain the difference only between last two revisions. Could you check the uploaded patch? You might change more things through multiple commits, but the submitted patch does not contain all changes. In your case, in order to get a collect patch, you should use the following commands: git fetch origin git diff --no-prefix origin/master > TAJO-1159.Jung.111114_2.patch.txt If you need more help, feel free to ask me anything.
        Hide
        jungjw Jaewoong Jung added a comment -

        Ok, here's my first patch for Tajo project.

        After examining jline console's code, I concluded that we could use the original history call chain as it is since it is designed to support bash-like behavior. So, I removed the addStatement method and relevant code in TajoCli and added filtering logic to the overriding add method. (And, some flag settings in TajoCli.initHistory)

        Please take a look and let me know if it makes sense to you.

        Show
        jungjw Jaewoong Jung added a comment - Ok, here's my first patch for Tajo project. After examining jline console's code, I concluded that we could use the original history call chain as it is since it is designed to support bash-like behavior. So, I removed the addStatement method and relevant code in TajoCli and added filtering logic to the overriding add method. (And, some flag settings in TajoCli.initHistory) Please take a look and let me know if it makes sense to you.

          People

          • Assignee:
            jungjw Jaewoong Jung
            Reporter:
            jungjw Jaewoong Jung
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development