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

[dilipbiswal]

What changes were proposed in this pull request?

In general we have a checkAnalysis phase which validates the logical plan and throws AnalysisException on semantic errors. However we also can throw AnalysisException from a few analyzer rules like ResolveSubquery.

I found that we fire up the analyzer rules twice for the queries that throw AnalysisException from one of the analyzer rules. This is a very minor fix. We don't have to strictly fix it. I just got confused seeing the rule getting fired two times when i was not expecting it.

How was this patch tested?

Tested manually.

[dilipbiswal]

cc @gatorsmile @cloud-fan Please let me know your thoughts.

[cloud-fan]

isn't analyzed a lazy val?

[dilipbiswal]

@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) ?

[cloud-fan]

isn't lazy val only be evaluated once?

[dilipbiswal]

@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 ?

[cloud-fan]

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

[dilipbiswal]

@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.

[cloud-fan]

put analyzed out of the try catch

[SparkQA]

Test build #74219 has finished for PR 17214 at commit 86582ef.

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

[hvanhovell]

You could make this a local lazy val. That should be a bit more concise. For example:

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

[dilipbiswal]

Thanks you @cloud-fan @hvanhovell. I will make the change.

[dilipbiswal]

@hvanhovell I tried the suggested code snippet. Actually since AnalyzedPlan is declared lazy the first evaluation happens inside the try block and hence it has the same problem. So lets just move analyzed outside the try block as wenchen suggests or change lazy val analyzedPlan = analyzed to val analyzedPlan = analyzed if you think it reads better ?

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

[hvanhovell]

Ahh, we want it materialized outside the block. Yeah, in that case wenchen's suggestion is the way to go

[dilipbiswal]

@hvanhovell Thank you.

[gatorsmile]

LGTM

[SparkQA]

Test build #74228 has finished for PR 17214 at commit 0dee6dd.

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

[gatorsmile]

Thanks! Merging to master/2.1

[dilipbiswal]

Thank you @gatorsmile @cloud-fan @hvanhovell