Skip to content

[WIP][SQL][TESTS] Disable stable column aliases in tests if assumed #51337

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.

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Jul 1, 2025

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

@github-actions github-actions bot added the SQL label Jul 1, 2025
@MaxGekk MaxGekk changed the title [WIP][SQL][TESTS] Disable stable column aliases in tests that it is assumed [WIP][SQL][TESTS] Disable stable column aliases in tests if assumed Jul 1, 2025
@github-actions github-actions bot added the AVRO label Jul 1, 2025
@@ -1762,7 +1762,9 @@ abstract class AvroSuite
}

test("error handling for unsupported Interval data types") {
withSQLConf(SQLConf.LEGACY_INTERVAL_ENABLED.key -> "true") {
withSQLConf(
SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED.key -> "false",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change the test query to add explicit alias?

Copy link
Member Author

@MaxGekk MaxGekk Jul 2, 2025

Choose a reason for hiding this comment

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

Yep, this could be another way, but this require much more work. I will try my best, and change some test in this way.

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 it's better to randomly switch this conf here and there. We just need to publish a good habit for writing tests: alias the expressions when necessary to have stable alias.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to randomly switch this conf here and there.

It is good idea, but before implementing this we should make sure that existing tests are reliable to such behaviour otherwise we will destabilize the existing CI.

We just need to publish a good habit for writing tests

Habits is nice, and I believe if a test writer assumes some naming convention, this assumption should be explicit via a config set otherwise he/she should assign an alias. How about to put this statement to the coding style?

alias the expressions when necessary to have stable alias.

I will do when it is obvious, but some tests are depend SQLConf.STABLE_DERIVED_COLUMN_ALIAS_ENABLED for unknown reasons at least for me (I cannot say why without investigation). For instance, the simple case where names should be the same in plan and in the query independently from the config:

  test("SPARK-42552: select and union without parentheses") {
    val plan = Distinct(OneRowRelation().select(Literal(1))
      .union(OneRowRelation().select(Literal(1))))
    assertEqual("select 1 union select 1", plan)
  }

fails with:

== FAIL: Plans do not match ===
 'Distinct                             'Distinct
 +- 'Union false, false                +- 'Union false, false
    :- 'Project [unresolvedalias(1)]      :- 'Project [unresolvedalias(1)]
    :  +- OneRowRelation                  :  +- OneRowRelation
    +- 'Project [unresolvedalias(1)]      +- 'Project [unresolvedalias(1)]
       +- OneRowRelation                     +- OneRowRelation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants