Skip to content

[SPARK-18106][SQL] ANALYZE TABLE should raise a ParseException for invalid option #15640

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.

Sign up for GitHub

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
dongjoon-hyun wants to merge 5 commits into from
Closed

[SPARK-18106][SQL] ANALYZE TABLE should raise a ParseException for invalid option #15640

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4819dd1
[SPARK-18106][SQL] ANALYZE TABLE should raise a ParseException for in…
dongjoon-hyun Oct 26, 2016
2a7707e
Add testcase.
dongjoon-hyun Oct 26, 2016
ec0516b
Check exception message.
dongjoon-hyun Oct 26, 2016
00b4f54
Address comments.
dongjoon-hyun Oct 30, 2016
465f646
Move partition spec warning to the top in order to cover all cases.
dongjoon-hyun Oct 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,10 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {
* }}}
*/
override def visitAnalyze(ctx: AnalyzeContext): LogicalPlan = withOrigin(ctx) {
if (ctx.partitionSpec == null &&
ctx.identifier != null &&
ctx.identifier.getText.toLowerCase == "noscan") {
if (ctx.identifier != null && ctx.identifier.getText.toLowerCase != "noscan") {

@hvanhovell hvanhovell Oct 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this check into the first if statement. There is no need to check this twice.

@hvanhovell hvanhovell Oct 30, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also think that the treatment of the partionSpec is quite funky. We scan the table as soon as a user defines a spec. Could you remove the null check; maybe it is better to just log a warning message and do what the user specified.

@dongjoon-hyun dongjoon-hyun Oct 30, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank you for review, @hvanhovell . I'll update the PR.

throw new ParseException(s"Expected `NOSCAN` instead of `${ctx.identifier.getText}`", ctx)
}
if (ctx.partitionSpec == null && ctx.identifier != null) {

@srinathshankar srinathshankar Oct 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if partition spec is not null ?
What happens with something like

ANALYZE TABLE mytable PARTITION (a) garbage

(Could you add a test for that ?)
Maybe

if (ctx.identifier != null && ctx.identifier.getText.toLowerCase != "noscan") {
  throw new ParseException(s"Expected `NOSCAN` instead of `${ctx.identifier.getText}`", ctx)
}

could be moved to the top ?

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank you for review and I'll handle that too.

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Test case is added.

AnalyzeTableCommand(visitTableIdentifier(ctx.tableIdentifier))
} else if (ctx.identifierSeq() == null) {
AnalyzeTableCommand(visitTableIdentifier(ctx.tableIdentifier), noscan = false)
Expand Down
13 changes: 11 additions & 2 deletions sql/core/src/test/scala/org/apache/spark/sql/execution/SparkSqlParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import org.apache.spark.sql.catalyst.catalog.{BucketSpec, CatalogStorageFormat,
import org.apache.spark.sql.catalyst.parser.ParseException
import org.apache.spark.sql.catalyst.plans.PlanTest
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.execution.command.{DescribeFunctionCommand, DescribeTableCommand,
ShowFunctionsCommand}
import org.apache.spark.sql.execution.command.{AnalyzeTableCommand, DescribeFunctionCommand,
DescribeTableCommand, ShowFunctionsCommand}
import org.apache.spark.sql.execution.datasources.{CreateTable, CreateTempViewUsing}
import org.apache.spark.sql.internal.{HiveSerDe, SQLConf}
import org.apache.spark.sql.types.{IntegerType, LongType, StringType, StructType}
Expand Down Expand Up @@ -220,4 +220,13 @@ class SparkSqlParserSuite extends PlanTest {

intercept("explain describe tables x", "Unsupported SQL statement")
}

test("SPARK-18106 analyze table") {

@srinathshankar srinathshankar Oct 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are also parse tests for AnalyzeTable in sql/hive/src/test/scala/org/apache/spark/sql/hive/StatisticsSuite.scala
Let's have these in the same place

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sure.

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ur, @srinathshankar .

I understand the reason why you put this there, so I looked at the StatisticsSuite.scala in both hive module and sql module.
But, we will not compare the value for this test case. If it's a parsing only grammar testcase, I prefer to put this in core.

How do you think about that?

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If you want, I'll remove the normal cases which raises no exceptions.

@srinathshankar srinathshankar Oct 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then maybe you can move those parse tests here ? All I'm suggesting is that the parse tests all be together.

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank you. But, the parse test should be rewritten. Is it okay? Those testcase uses assertAnalyzeCommand using SparkSession and looks like the following.

    def assertAnalyzeCommand(analyzeCommand: String, c: Class[_]) {
      val parsed = spark.sessionState.sqlParser.parsePlan(analyzeCommand)
      val operators = parsed.collect {
        case a: AnalyzeTableCommand => a
        case o => o
      }

      assert(operators.size === 1)
      if (operators(0).getClass() != c) {
        fail(
          s"""$analyzeCommand expected command: $c, but got ${operators(0)}
             |parsed command:
             |$parsed
           """.stripMargin)
      }
    }

    assertAnalyzeCommand(
      "ANALYZE TABLE Table1 COMPUTE STATISTICS",
      classOf[AnalyzeTableCommand])

@srinathshankar srinathshankar Oct 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In my opinion it's fine to rewrite and simplify. If you could do that, that would be great.

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep. I have no objection for that. Actually, I love to do that.
But, I'd like to wait for some directional advice from committer.

@hvanhovell hvanhovell Oct 30, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lets keep the parsing unit test (this file) and the analyze table integration test separate for now.

@dongjoon-hyun dongjoon-hyun Oct 30, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank you for the guide!

assertEqual("analyze table t compute statistics",
AnalyzeTableCommand(TableIdentifier("t"), noscan = false))
assertEqual("analyze table t compute statistics noscan",
AnalyzeTableCommand(TableIdentifier("t"), noscan = true))
intercept("analyze table t compute statistics xxxx", "Expected `NOSCAN` instead of `xxxx`")
intercept("analyze table t partition (a) compute statistics xxxx")

@srinathshankar srinathshankar Oct 26, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: , "ExpectedNOSCANinstead ofxxxx" here as well.

@dongjoon-hyun dongjoon-hyun Oct 26, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank you, It's done.

}
}