Skip to content

Conversation

@miland-db
Copy link
Contributor

@miland-db miland-db commented Nov 7, 2024

What changes were proposed in this pull request?

We are introducing checks for unique label names.
New rules for label names:

  • Labels can't have the same name as some of the labels in scope surrounding them
  • Labels can have the same name as other labels in the same scope

Valid code:

BEGIN
  lbl: BEGIN
    SELECT 1;
  END;

  lbl: BEGIN
    SELECT 2;
  END;

  BEGIN
    lbl: WHILE 1=1 DO
      LEAVE lbl;
    END WHILE;
  END;
END

Invalid code:

BEGIN
  lbl: BEGIN
    lbl: BEGIN
      SELECT 1;
    END;
  END;
END

Design explanation:

Even though there are Listeners with enterRule and exitRule methods to check labels before and remove them from seenLabels after visiting node, we favor this approach because minimal changes were needed and code is more compact to avoid dependency issues.

Additionally, generating label text would need to be done in 2 places and we wanted to avoid duplicated logic:

  • enterRule
  • visitRule

Why are the changes needed?

It will be needed in future when we release Local Scoped Variables for SQL Scripting so users can target variables from outer scopes if they are shadowed.

How was this patch tested?

New unit tests in 'SqlScriptingParserSuite.scala'.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Nov 7, 2024
@miland-db
Copy link
Contributor Author

@miland-db miland-db changed the title [SPARK-49913] Add check for unique label names in nested labeled scopes [SPARK-49913][SQL] Add check for unique label names in nested labeled scopes Nov 7, 2024
Copy link
Contributor

@davidm-db davidm-db left a comment

Choose a reason for hiding this comment

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

Please make seenLabels thread local as Wenchen suggested, other than that LGTM.


override def visitSingleCompoundStatement(ctx: SingleCompoundStatementContext): CompoundBody = {
visit(ctx.beginEndCompoundBlock()).asInstanceOf[CompoundBody]
val seenLabels = Set[String]()
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this is the implementation I was expecting. Can we do a bit more encapsulation? e.g.

val labelCtx = new ScritpingLabelContext()
visitBeginEndCompoundBlockImpl...
...
class ScritpingLabelContext {
  private val seenLabels = Set[String]()
  
  def newLabel(label: String)...
  ...
}
``

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cloud-fan done.

@miland-db miland-db requested a review from cloud-fan November 13, 2024 11:18
Copy link
Contributor

@davidm-db davidm-db left a comment

Choose a reason for hiding this comment

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

Just a tiny comment, otherwise LGTM.
I would maybe use SqlScriptingLabelContext since we used SqlScripting* I think everywhere, so just to keep it consistent.

@cloud-fan
Copy link
Contributor

The linter failure is unrelated, thanks, merging to master!

@cloud-fan cloud-fan closed this in 2fd4702 Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants