Skip to content

[SPARK-4935][SQL] When hive.cli.print.header configured, spark-sql aborted if passed in a invalid sql #3761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

scwf
Copy link
Contributor

@scwf scwf commented Dec 22, 2014

If we passed in a wrong sql like abdcdfsfs, the spark-sql script aborted.

@SparkQA
Copy link

SparkQA commented Dec 22, 2014

Test build #24701 has started for PR 3761 at commit 1614a11.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 22, 2014

Test build #24703 has started for PR 3761 at commit 0330e07.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 22, 2014

Test build #24701 has finished for PR 3761 at commit 1614a11.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24701/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Dec 22, 2014

Test build #24703 has finished for PR 3761 at commit 0330e07.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24703/
Test PASSed.

@liancheng
Copy link
Contributor

Hm, just tried spark-sql script in both Spark 1.2.0 and the most recent master, passing an invalid SQL doesn't make the script abort...

@scwf
Copy link
Contributor Author

scwf commented Dec 23, 2014

Hi @liancheng, this is a very interesting bug i think, if you place a hive-site.xml into spark/conf, this will happen.

@scwf
Copy link
Contributor Author

scwf commented Dec 23, 2014

If we config

<property>
  <name>hive.cli.print.header</name>
  <value>true</value>
</property>

spark-sql will abort.

@scwf
Copy link
Contributor Author

scwf commented Dec 23, 2014

This is because sparksqldriver always return ResponseCode(0), even if we pass a invalid sql. So

     if (ret != 0) {
            console.printError(rc.getErrorMessage())
            driver.close()
            return ret
          }

will be never executed.
And after we config hive.cli.print.header

if (HiveConf.getBoolVar(conf, HiveConf.ConfVars.HIVE_CLI_PRINT_HEADER)) {
            // Print the column names.
            Option(driver.getSchema.getFieldSchemas).map { fields =>
              out.println(fields.map(_.getName).mkString("\t"))
            }
          }

it will throw NPE when passed in a invalid sql

@scwf scwf changed the title [SQL] spark-sql aborted if passed in a wrong sql [SQL] When hive.cli.print.header configured, spark-sql aborted if passed in a invalid sql Dec 23, 2014
@scwf scwf changed the title [SQL] When hive.cli.print.header configured, spark-sql aborted if passed in a invalid sql [SPARK-4935][SQL] When hive.cli.print.header configured, spark-sql aborted if passed in a invalid sql Dec 23, 2014
@liancheng
Copy link
Contributor

@scwf Thanks for the explanation! Then this change makes sense and LGTM.

@@ -278,7 +278,6 @@ private[hive] class SparkSQLCLIDriver extends CliDriver with Logging {

ret = rc.getResponseCode
if (ret != 0) {
console.printError(rc.getErrorMessage())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I don't think we should remove this line, although this may cause duplicated error reporting in this specific case, removing this may silent other error messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually before this PR this line do not executed, seems it does not matter, can you explain why it will silent some error messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not triggered because we didn't assign non-zero return code properly before. But now we're using proper return code. Although exceptions and error messages are also caught and reported in other places (e.g. within HiveContext), I still tend to keep this line.

@SparkQA
Copy link

SparkQA commented Dec 24, 2014

Test build #24754 has started for PR 3761 at commit 46dc344.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 24, 2014

Test build #24754 has finished for PR 3761 at commit 46dc344.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24754/
Test PASSed.

@scwf
Copy link
Contributor Author

scwf commented Dec 24, 2014

Updated.

@marmbrus
Copy link
Contributor

Thanks! Merged to master.

@asfgit asfgit closed this in 8f29b7c Dec 30, 2014
@scwf scwf deleted the patch-10 branch January 8, 2015 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants