Hive
  1. Hive
  2. HIVE-2525

Introduce proper error messages, when explain fails on commands that otherwise could be succesful (commands that are not analyzed by Semantic Analyzer)

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Trivial Trivial
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    1. HIVE-2525.1.patch
      2 kB
      Robert Surówka

      Activity

      Hide
      Robert Surówka added a comment -
      Show
      Robert Surówka added a comment - Diff request at https://reviews.apache.org/r/2561/diff/#index_header
      Hide
      Namit Jain added a comment -

      hive> explain dfs -ls <PATH>;
      FAILED: Parse Error: line 1:8 cannot recognize input near 'dfs' '-' 'ls' in statement

      The above error is a legitimate parse error.
      If we want to fix it with a better error message, we should catch the parser error and throw
      a beter message (the way you have put it in CliDriver). The approach you have taken is difficult
      to maintain in the long run.

      Show
      Namit Jain added a comment - hive> explain dfs -ls <PATH>; FAILED: Parse Error: line 1:8 cannot recognize input near 'dfs' '-' 'ls' in statement The above error is a legitimate parse error. If we want to fix it with a better error message, we should catch the parser error and throw a beter message (the way you have put it in CliDriver). The approach you have taken is difficult to maintain in the long run.
      Hide
      Robert Surówka added a comment -

      I think such an error is incorrect since "dfs -ls <PATH>" (without explain) would never throw such an error on it's own (and execution of "dfs..." doesn't go to semantic analysis), so I see no reason for explain of "dfs...." to go to semantic analysis and throw semantic analysis type error.

      Furthermore having check for incorrect explains at the beginning saves a lot of time, otherwise all the analysis and various class instances would be need to be created only to find error that we can easily see from the very beginning. And if someone would like to add or remove a command that doesn't go to Driver to be compiled, he will need to add or remove an "if" in CliDriver anyway so it shouldn't be hard for him to notice that he also needs to update that list. Finally if that suggestion of fix will pass I can add appropriate tests, checking if explains errors of non-sematic-analisys commands are informative, so it will be easier to see if something is wrong after subsequent changes.

      The bad thing is that after this change, even for valid explain query we will check "contains" on that list, but fortunately since it is short it doesn't cost too much time.

      Show
      Robert Surówka added a comment - I think such an error is incorrect since "dfs -ls <PATH>" (without explain) would never throw such an error on it's own (and execution of "dfs..." doesn't go to semantic analysis), so I see no reason for explain of "dfs...." to go to semantic analysis and throw semantic analysis type error. Furthermore having check for incorrect explains at the beginning saves a lot of time, otherwise all the analysis and various class instances would be need to be created only to find error that we can easily see from the very beginning. And if someone would like to add or remove a command that doesn't go to Driver to be compiled, he will need to add or remove an "if" in CliDriver anyway so it shouldn't be hard for him to notice that he also needs to update that list. Finally if that suggestion of fix will pass I can add appropriate tests, checking if explains errors of non-sematic-analisys commands are informative, so it will be easier to see if something is wrong after subsequent changes. The bad thing is that after this change, even for valid explain query we will check "contains" on that list, but fortunately since it is short it doesn't cost too much time.
      Hide
      Ning Zhang added a comment -

      I agree with Namit. The right approach is to throw a better error message in the parser rather than adding a special checking for a specific negative case. There are a lot of such negative cases. If we add checks for all of them, it becomes cumbersome and redundant in terms of parser's functionality.

      Show
      Ning Zhang added a comment - I agree with Namit. The right approach is to throw a better error message in the parser rather than adding a special checking for a specific negative case. There are a lot of such negative cases. If we add checks for all of them, it becomes cumbersome and redundant in terms of parser's functionality.

        People

        • Assignee:
          Unassigned
          Reporter:
          Robert Surówka
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:

            Development