Skip to content

[MINOR][SQL] The analyzer rules are fired twice for cases when AnalysisException is raised from analyzer. #17214

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
dilipbiswal wants to merge 2 commits into from
Closed

[MINOR][SQL] The analyzer rules are fired twice for cases when AnalysisException is raised from analyzer. #17214

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86582ef
[SQL][MINOR] The analyzer rules are fired twice for cases AnalysisExc…
dilipbiswal Mar 8, 2017
0dee6dd
Review comments
dilipbiswal Mar 8, 2017
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
9 changes: 7 additions & 2 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,14 @@ class QueryExecution(val sparkSession: SparkSession, val logical: LogicalPlan) {
protected def planner = sparkSession.sessionState.planner

def assertAnalyzed(): Unit = {
try sparkSession.sessionState.analyzer.checkAnalysis(analyzed) catch {
// Analyzer is invoked outside the try block to avoid calling it again from within the
Copy link
Contributor

@cloud-fan cloud-fan Mar 8, 2017

Choose a reason for hiding this comment

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

isn't analyzed a lazy val?

Copy link
Contributor Author

@dilipbiswal dilipbiswal Mar 8, 2017

Choose a reason for hiding this comment

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

@cloud-fan Yeah wenchen. so first time we invoke analyzer is on this line. And upon exception we go to catch block -

     case e: AnalysisException =>
       val ae = new AnalysisException(e.message, e.line, e.startPosition, Some(analyzed))

and call analyzer the second time while trying to evaluate the last parameter - Some(analyzed) ?

Copy link
Contributor

@cloud-fan cloud-fan Mar 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

isn't lazy val only be evaluated once?

Copy link
Contributor Author

@dilipbiswal dilipbiswal Mar 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@cloud-fan Actually thats what i also thought Wenchen. But it seems like if an exception occurred before the assignment to lazy val happens , then it treats it like the first evaluation never happened ?

Copy link
Contributor

@cloud-fan cloud-fan Mar 8, 2017

Choose a reason for hiding this comment

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

so we just need a line before this line, to materialize analyzed.

Copy link
Contributor Author

@dilipbiswal dilipbiswal Mar 8, 2017

Choose a reason for hiding this comment

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

@cloud-fan did you mean like this ?

def assertAnalyzed(): Unit = {
    try {
      analyzed
      sparkSession.sessionState.analyzer.checkAnalysis(analyzed)
    } catch {
      case e: AnalysisException =>
        val ae = new AnalysisException(e.message, e.line, e.startPosition, Some(analyzed))
        ae.setStackTrace(e.getStackTrace)
        throw ae
    }
  }

If so, it also causes two invocation like before.

Copy link
Contributor

@cloud-fan cloud-fan Mar 8, 2017

Choose a reason for hiding this comment

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

put analyzed out of the try catch

// catch block below.
analyzed
try {
sparkSession.sessionState.analyzer.checkAnalysis(analyzed)
} catch {
case e: AnalysisException =>
val ae = new AnalysisException(e.message, e.line, e.startPosition, Some(analyzed))
val ae = new AnalysisException(e.message, e.line, e.startPosition, Option(analyzed))
ae.setStackTrace(e.getStackTrace)
throw ae
}
Expand Down