Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.12.0
    • Component/s: grunt
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      This feature is aimed at providing shortcuts for frequently used commands like illustrate, dump, explain, describe, quit, help etc. This feature is inspired from postgres(psql) shortcuts. I tried implementing a simple shortcut for quitting the grunt shell using \q with very minimal changes. I think this feature will help save many keystrokes for users. If this feature looks useful I can submit the current patch for review and go ahead with implementing the following shortcuts

      \i <alias> - illustrate
      \e <alias> - explain
      \de <alias> - describe
      \du <alias> - dump
      \h - help

      This will also be useful to view information about tables/statistics stored in HCatalog similar to the way psql does.
      \dt <alias> - display table
      \dm - display metadata
      etc..

      except \t, \r and \n delimiters we should be able to use all other characters as shortcuts.
      Please let me know your thoughts.

      1. PIG-2994.1.git.patch
        13 kB
        Prasanth J
      2. PIG-2994.2.git.patch
        17 kB
        Prasanth J
      3. PIG-2994.3.git.patch
        17 kB
        Prasanth J
      4. PIG-2994.hotfix.patch
        1.0 kB
        Cheolsoo Park

        Issue Links

          Activity

          Hide
          Prashant Kommireddi added a comment -

          Nice, I think this would be useful. Like you said backslash (with t,r,n) might be confused with the tab, newline delimiters. How about using a colon instead?

          Show
          Prashant Kommireddi added a comment - Nice, I think this would be useful. Like you said backslash (with t,r,n) might be confused with the tab, newline delimiters. How about using a colon instead?
          Hide
          Prasanth J added a comment -

          The reason I thought of using \ instead of other char is that, it will be easy for postgres users(not sure if any other databases support shell shortcuts) to easily adapt to this convention (colon shouldn't be hard though). This convention will be more useful for viewing/exploring HCat tables, and for frequently used diagnostic operators (dump, describe, explain, illustrate). Since these operators doesn't start with t,r,n, IMHO it will not be confused with tab, newline.

          Show
          Prasanth J added a comment - The reason I thought of using \ instead of other char is that, it will be easy for postgres users(not sure if any other databases support shell shortcuts) to easily adapt to this convention (colon shouldn't be hard though). This convention will be more useful for viewing/exploring HCat tables, and for frequently used diagnostic operators (dump, describe, explain, illustrate). Since these operators doesn't start with t,r,n, IMHO it will not be confused with tab, newline.
          Hide
          Prashant Kommireddi added a comment -

          I support having keyboard shortcuts for Pig. Committers, thoughts?

          Show
          Prashant Kommireddi added a comment - I support having keyboard shortcuts for Pig. Committers, thoughts?
          Hide
          Prasanth J added a comment -

          The attached patch adds grunt shortcuts for most commonly used diagnostic commands. Following shortcuts were used for diagnostic commands

          \i - illustrate
          \d - dump
          \de - describe
          \e - explain

          For all the above commands if alias is ignored the last defined alias will be used.

          Also noticed that the default implementation throws error when alias is ignored for illustrate and explain commands. This patch modifies it to use the last defined alias when alias is ignored. Is there any reason why the default implementation (processIllustrate and processExplain) doesn't consider the last alias when alias is ignored?

          Additionally, \q shortcut is used for quitting the grunt shell.

          Show
          Prasanth J added a comment - The attached patch adds grunt shortcuts for most commonly used diagnostic commands. Following shortcuts were used for diagnostic commands \i - illustrate \d - dump \de - describe \e - explain For all the above commands if alias is ignored the last defined alias will be used. Also noticed that the default implementation throws error when alias is ignored for illustrate and explain commands. This patch modifies it to use the last defined alias when alias is ignored. Is there any reason why the default implementation (processIllustrate and processExplain) doesn't consider the last alias when alias is ignored? Additionally, \q shortcut is used for quitting the grunt shell.
          Hide
          Prasanth J added a comment -

          Ping! Can someone please review this patch? Not sure to whom I should send review request in RB.

          Show
          Prasanth J added a comment - Ping! Can someone please review this patch? Not sure to whom I should send review request in RB.
          Hide
          Cheolsoo Park added a comment -

          Hi Prasanth,

          Sorry for the delay. I will review it today.

          In the meantime, do you mind updating docs? I think we can add a table of shortcuts to this section: http://pig.apache.org/docs/r0.10.0/start.html#interactive-mode. What do you think?

          Show
          Cheolsoo Park added a comment - Hi Prasanth, Sorry for the delay. I will review it today. In the meantime, do you mind updating docs? I think we can add a table of shortcuts to this section: http://pig.apache.org/docs/r0.10.0/start.html#interactive-mode . What do you think?
          Hide
          Prasanth J added a comment -

          Hi Cheolsoo

          Yeah. I will update the docs as well. Since the shortcuts will work in grunt shell and pig scripts (sorry for the misleading JIRA topic), I feel its more appropriate to add documentation under Pig Latin debug section. http://pig.apache.org/docs/r0.10.0/start.html#debug

          Any thoughts?

          Show
          Prasanth J added a comment - Hi Cheolsoo Yeah. I will update the docs as well. Since the shortcuts will work in grunt shell and pig scripts (sorry for the misleading JIRA topic), I feel its more appropriate to add documentation under Pig Latin debug section. http://pig.apache.org/docs/r0.10.0/start.html#debug Any thoughts?
          Hide
          Cheolsoo Park added a comment -

          I like your suggestion better. Thanks!

          Show
          Cheolsoo Park added a comment - I like your suggestion better. Thanks!
          Hide
          Cheolsoo Park added a comment -

          Prasanth J Thank you very much for the patch! This is very convenient. I have two suggestions.

          • Can we simplify the grammar?
            From:
                <DUMP>
                (
                    t1 = <IDENTIFIER>
                    {processDump(t1.image);}
                    |
                    {processDump(null);}
                )
                |
                <DUMP_SHORT>
                (
                    t1 = <IDENTIFIER>
                    {processDump(t1.image);}
                    |
                    {processDump(null);}
                )
            

            To:

                ( <DUMP> | <DUMP_SHORT> )
                (
                    t1 = <IDENTIFIER>
                    {processDump(t1.image);}
                    |
                    {processDump(null);}
                )
            

            The same goes to all the shortcuts that you're adding. I tested it myself, and it seems to work. Please correct me if I am wrong because I am no expert on JAVACC.

          • Can we throw an exception if dump/explain/illustrate is called with no alias && there is no previous alias in the Pig context?
            For example, if I have a dummy script such as follows:
            DUMP;
            

            Since dump, explain, and illustrate are now allowed with no alias, my script fails with the following error at logical plan validation:

            org.apache.pig.impl.logicalLayer.FrontendException: ERROR 1066: Unable to open iterator for alias null
            

            However, it used to fail faster at parsing:

            ERROR 1000: Error during parsing. Encountered " <IDENTIFIER> "d "" at line 2, column 1.
            

            I think the original error is more intuitive. We can probably add a null check after getLastAlias(). For example,

            alias = mPigServer.getPigContext().getLastAlias();
            if (alias == null) {
                throw new ParseException("'illustrate' statement must be on an alias or on a script.");
            }
            

          Please let me know what you think. Thanks!

          Show
          Cheolsoo Park added a comment - Prasanth J Thank you very much for the patch! This is very convenient. I have two suggestions. Can we simplify the grammar? From: <DUMP> ( t1 = <IDENTIFIER> {processDump(t1.image);} | {processDump( null );} ) | <DUMP_SHORT> ( t1 = <IDENTIFIER> {processDump(t1.image);} | {processDump( null );} ) To: ( <DUMP> | <DUMP_SHORT> ) ( t1 = <IDENTIFIER> {processDump(t1.image);} | {processDump( null );} ) The same goes to all the shortcuts that you're adding. I tested it myself, and it seems to work. Please correct me if I am wrong because I am no expert on JAVACC. Can we throw an exception if dump/explain/illustrate is called with no alias && there is no previous alias in the Pig context? For example, if I have a dummy script such as follows: DUMP; Since dump, explain, and illustrate are now allowed with no alias, my script fails with the following error at logical plan validation: org.apache.pig.impl.logicalLayer.FrontendException: ERROR 1066: Unable to open iterator for alias null However, it used to fail faster at parsing: ERROR 1000: Error during parsing. Encountered " <IDENTIFIER> " d "" at line 2, column 1. I think the original error is more intuitive. We can probably add a null check after getLastAlias(). For example, alias = mPigServer.getPigContext().getLastAlias(); if (alias == null ) { throw new ParseException( "'illustrate' statement must be on an alias or on a script." ); } Please let me know what you think. Thanks!
          Hide
          Prasanth J added a comment -

          Great suggestions Cheolsoo! Thanks for your review.

          Regarding your 2nd suggestion, one thing I noticed from the grunt parser code is that only ILLUSTRATE and EXPLAIN operator supports "-script" option wherein we can pass a pig script as an argument. Hence these commands are valid even when last alias is not defined yet.
          For example:

          grunt> \i -script /illustrate.pig

          is a valid operation even when there is no previously defined alias. Similarly for explain.

          Hence just checking 'alias == null' after will make the above operation invalid. So my suggestion is to do the following

          alias = mPigServer.getPigContext().getLastAlias();
          if (alias == null && script == null) {
              throw new ParseException("'illustrate' statement must be on an alias or on a script.");
          }

          Please let me know if this looks good. I will resubmit the patch incorporating the changes you had mentioned along with documentation.

          Show
          Prasanth J added a comment - Great suggestions Cheolsoo! Thanks for your review. Regarding your 2nd suggestion, one thing I noticed from the grunt parser code is that only ILLUSTRATE and EXPLAIN operator supports "-script" option wherein we can pass a pig script as an argument. Hence these commands are valid even when last alias is not defined yet. For example: grunt> \i -script /illustrate.pig is a valid operation even when there is no previously defined alias. Similarly for explain. Hence just checking 'alias == null' after will make the above operation invalid. So my suggestion is to do the following alias = mPigServer.getPigContext().getLastAlias(); if (alias == null && script == null ) { throw new ParseException( "'illustrate' statement must be on an alias or on a script." ); } Please let me know if this looks good. I will resubmit the patch incorporating the changes you had mentioned along with documentation.
          Hide
          Cheolsoo Park added a comment -

          Sounds good to me. Thanks for the quick response!

          Show
          Cheolsoo Park added a comment - Sounds good to me. Thanks for the quick response!
          Hide
          Prasanth J added a comment -

          Hi Cheolsoo

          Submitted the new patch with the changes incorporated. Added few more testcases and updated documentation. Please let me know if the patch looks good.

          Show
          Prasanth J added a comment - Hi Cheolsoo Submitted the new patch with the changes incorporated. Added few more testcases and updated documentation. Please let me know if the patch looks good.
          Hide
          Cheolsoo Park added a comment -

          Prasanth J "ant clean docs" doesn't compile for me. I am using Forrest-0.9. Do you mind fixing it?

               [exec] validate-xdocs:
               [exec] /home/cheolsoo/workspace/pig-git/src/docs/src/documentation/content/xdocs/start.xml:395:8: Element type "h4" must be declared.
               [exec] /home/cheolsoo/workspace/pig-git/src/docs/src/documentation/content/xdocs/start.xml:414:11: The content of element type "section" must match "(title,(section|p|source|note|warning|fixme|table|ol|ul|dl|figure|anchor|xi:include)*)".
          

          Other than that, I am +1. I will commit it as soon as you fix the doc compile error.

          Thanks!

          Show
          Cheolsoo Park added a comment - Prasanth J "ant clean docs" doesn't compile for me. I am using Forrest-0.9. Do you mind fixing it? [exec] validate-xdocs: [exec] /home/cheolsoo/workspace/pig-git/src/docs/src/documentation/content/xdocs/start.xml:395:8: Element type "h4" must be declared. [exec] /home/cheolsoo/workspace/pig-git/src/docs/src/documentation/content/xdocs/start.xml:414:11: The content of element type "section" must match "(title,(section|p|source|note|warning|fixme|table|ol|ul|dl|figure|anchor|xi:include)*)" . Other than that, I am +1. I will commit it as soon as you fix the doc compile error. Thanks!
          Hide
          Prasanth J added a comment -

          Cheolsoo Park I ran "forrest run" when I tested the doc which rendered the page correctly. I wasn't aware of "ant clean docs". Sorry about that. The updated patch should fix it. Thanks.

          Show
          Prasanth J added a comment - Cheolsoo Park I ran "forrest run" when I tested the doc which rendered the page correctly. I wasn't aware of "ant clean docs". Sorry about that. The updated patch should fix it. Thanks.
          Hide
          Cheolsoo Park added a comment -

          Committed to trunk. Thanks Prasanth for the contribution!

          Show
          Cheolsoo Park added a comment - Committed to trunk. Thanks Prasanth for the contribution!
          Hide
          Prasanth J added a comment -

          Thanks Cheolsoo for committing.

          Show
          Prasanth J added a comment - Thanks Cheolsoo for committing.
          Hide
          Cheolsoo Park added a comment -

          Two tests are broken by this patch:

          • TestGrunt
          • TestMultiQueryBasic

          I linked jiras for the failing test cases.

          Show
          Cheolsoo Park added a comment - Two tests are broken by this patch: TestGrunt TestMultiQueryBasic I linked jiras for the failing test cases.
          Hide
          Cheolsoo Park added a comment -

          While debugging TestGrunt, I came to conclude that the fix should be made to the patch of this jira.

          PIG-2994.3.patch introduced a backward incompatible behavior to explain when it being used in PigServer.

          Originally, explain with no alias followed by a script name used to perform explain on the script. But now it tries to perform explain on the previous alias.

          The following change restores the original behavior. I verified that this fixes the test failures in TestGrunt:

          -        if (alias == null) {
          +        if (alias == null && script == null) {
                       alias = mPigServer.getPigContext().getLastAlias();
                       // if explain is used immediately after launching grunt shell then
                       // last defined alias will be null
          -            if (alias == null && script == null) {
          +            if (alias == null) {
                           throw new ParseException("'explain' statement must be on an alias or on a script.");
                       }
          
          Show
          Cheolsoo Park added a comment - While debugging TestGrunt, I came to conclude that the fix should be made to the patch of this jira. PIG-2994 .3.patch introduced a backward incompatible behavior to explain when it being used in PigServer. Originally, explain with no alias followed by a script name used to perform explain on the script. But now it tries to perform explain on the previous alias. The following change restores the original behavior. I verified that this fixes the test failures in TestGrunt: - if (alias == null ) { + if (alias == null && script == null ) { alias = mPigServer.getPigContext().getLastAlias(); // if explain is used immediately after launching grunt shell then // last defined alias will be null - if (alias == null && script == null ) { + if (alias == null ) { throw new ParseException( "'explain' statement must be on an alias or on a script." ); }
          Hide
          Prasanth J added a comment -

          Hi Chelosoo

          Does this patch fix both the issues? If not I can look into the other issue.

          Show
          Prasanth J added a comment - Hi Chelosoo Does this patch fix both the issues? If not I can look into the other issue.
          Hide
          Cheolsoo Park added a comment -

          Hi Prasanth,

          My hotfix patch in this jira only fixes TestGrunt. I also uploaded a separate fix to TestMultiQueryBasic in PIG-3168.

          It would be nice if you could take a look at them. Please let me know if you have a better suggestion on how to fix them.

          Thanks!

          Show
          Cheolsoo Park added a comment - Hi Prasanth, My hotfix patch in this jira only fixes TestGrunt. I also uploaded a separate fix to TestMultiQueryBasic in PIG-3168 . It would be nice if you could take a look at them. Please let me know if you have a better suggestion on how to fix them. Thanks!
          Hide
          Cheolsoo Park added a comment -

          Can anyone review "PIG-2994.hotfix.patch" please? This will fix TestGrunt in trunk.

          Thank you!

          Show
          Cheolsoo Park added a comment - Can anyone review " PIG-2994 .hotfix.patch" please? This will fix TestGrunt in trunk. Thank you!
          Hide
          Jonathan Coveney added a comment -

          +1

          Show
          Jonathan Coveney added a comment - +1
          Hide
          Cheolsoo Park added a comment -

          Committed hotfix to trunk.

          Thank you Jonathan for the review!

          Show
          Cheolsoo Park added a comment - Committed hotfix to trunk. Thank you Jonathan for the review!
          Hide
          Cheolsoo Park added a comment -

          Updating fix version.

          Show
          Cheolsoo Park added a comment - Updating fix version.

            People

            • Assignee:
              Prasanth J
              Reporter:
              Prasanth J
            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development