Skip to content

Conversation

@mihailoale-db
Copy link
Contributor

What changes were proposed in this pull request?

Refactoring of ResolveSQLOnFile rule.

Why are the changes needed?

Refactoring is needed for the Single-pass Analyzer project (please check link)

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests.

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

No

@github-actions github-actions bot added the SQL label Nov 19, 2024
Copy link
Contributor

@dtenedor dtenedor left a comment

Choose a reason for hiding this comment

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

LGTM after resolving the one comment

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have to put this object outside of the class if you want any other code to be able to use it? Playing around with the Scala REPL, I am not able to refer to the nested object without creating an instance of the class.

Welcome to the Ammonite Repl 3.0.0-M2 (Scala 3.3.3 Java 17.0.12)
@ class c() {
    object d {
      val x = 42
    }
  } 
defined class c

@ val e = new c() 
e: c = ammonite.$sess.cmd0$c@7a48e6e2

@ e.d 
res2: _root_.ammonite.$sess.cmd1.e.d.type = ammonite.$sess.cmd0$c$d$@9d2723e

@ e.d.x 
res3: Int = 42

@ c.d.x 
-- [E008] Not Found Error: cmd4.sc:1:13 ----------------------------------------
1 |val res4 = c.d.x
  |           ^^^
  |value d is not a member of object ammonite.$sess.cmd0.c, but could be made available as an extension method.
  |
  |The following import might make progress towards fixing the problem:
  |
  |  import sourcecode.Text.generate
  |
Compilation Failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we move it out of class we'll have to introduce some other changes e.g make maybeSQLFile public etc. I think it is better like this (we create an instance of class and use the object as a field of it). @dtenedor

@mihailoale-db mihailoale-db force-pushed the mihailoale-db/refactorresolvesqlonfile branch from f3c3a4e to e88f491 Compare November 20, 2024 09:18
@MaxGekk MaxGekk changed the title [SPARK-50353] [SQL] Refactor ResolveSQLOnFile [SPARK-50353][SQL] Refactor ResolveSQLOnFile Nov 20, 2024
@MaxGekk
Copy link
Member

MaxGekk commented Nov 20, 2024

+1, LGTM. Merging to master.
Thank you, @mihailoale-db and @dtenedor for review.

@MaxGekk MaxGekk closed this in 23f276f Nov 20, 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.

3 participants