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 all 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
10 changes: 7 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,13 @@ 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.partitionSpec != null) {
logWarning(s"Partition specification is ignored: ${ctx.partitionSpec.getText}")
}
if (ctx.identifier != null) {
if (ctx.identifier.getText.toLowerCase != "noscan") {
throw new ParseException(s"Expected `NOSCAN` instead of `${ctx.identifier.getText}`", ctx)
}
AnalyzeTableCommand(visitTableIdentifier(ctx.tableIdentifier))
} else if (ctx.identifierSeq() == null) {
AnalyzeTableCommand(visitTableIdentifier(ctx.tableIdentifier), noscan = false)
Expand Down
18 changes: 16 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,18 @@ class SparkSqlParserSuite extends PlanTest {

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

test("SPARK-18106 analyze table") {
Copy link
Contributor

@srinathshankar srinathshankar Oct 26, 2016

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

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

Choose a reason for hiding this comment

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

Sure.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

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?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

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.

Copy link
Contributor

@srinathshankar srinathshankar Oct 26, 2016

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.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

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])

Copy link
Contributor

@srinathshankar srinathshankar Oct 26, 2016

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.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 26, 2016

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.

Copy link
Contributor

@hvanhovell hvanhovell Oct 30, 2016

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.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Oct 30, 2016

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))
assertEqual("analyze table t partition (a) 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",
"Expected `NOSCAN` instead of `xxxx`")
}
}