[SPARK-54412][SQL][TEST] Clean up `v` properly in `identifier-clause.sql` SQL golden file #53121

vrozov · 2025-11-18T22:53:42Z

What changes were proposed in this pull request?

Drop view v in identifier-clause.sql

Why are the changes needed?

After #52765 view v is created and it is necessary to cleanup this view to avoid conflicts

Does this PR introduce any user-facing change?

No

How was this patch tested?

updated golden files and run the test

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

No

…sql golden file

vrozov · 2025-11-18T22:54:52Z

@srielau and @cloud-fan Please review.

vrozov · 2025-11-18T22:58:05Z

In my local environment ThriftServerQueryTestSuite was failing during describe.sql execution (failed to create view v) after identifier-clause.sql.

cloud-fan · 2025-11-19T03:21:55Z

sql/core/src/test/resources/sql-tests/inputs/identifier-clause.sql

 WITH identifier('v')(identifier('c1')) AS (VALUES(1)) (SELECT c1 FROM v);
 CREATE OR REPLACE VIEW v(IDENTIFIER('c1')) AS VALUES(1);
 SELECT c1 FROM v;
+DROP VIEW v;


Suggested change

DROP VIEW v;

DROP VIEW IF EXISTS v;

That's weird. Why could this be needed) There is something else hiding here. Unless the test files are executed in parallel. In that case we have a bigger problem.

Oh never mind.. The DROP VIEW was missing.

@cloud-fan I don't think that IF EXIST is preferred in this case

view is created just 2 lines above

if create view errors out, it is expected that select and drop would also error out

all drop view in this file do not use IF EXIST

My point is to not let DROP VIEW error out. It's just for cleanup the test resource, there is no point to test its error behavior.

It does not matter here if DROP VIEW v errors out or not. The goal is to cleanup and if the view does not exist (in the legacy case), both SELECT and DROP would error out consistently. The error does not affect anything.

The error gets committed into the golden file...

The same for SELECT one line above.

The cleanup code should be as less noisy as possible. Maybe we should have a clear separation between testing code and cleanup code in the golden file test framework, so that we don't commit anything for the cleanup code in the golden file. The SELECT one line above is clearly a testing code.

Thanks @dongjoon-hyun for quickly addressing this issue properly!

dongjoon-hyun

Thank you, @vrozov . It would be great if you can address @cloud-fan 's comment if you want to move forward.

dongjoon-hyun

Hi, @vrozov , @cloud-fan , @srielau .

As a release manager, I made an alternative PR while keeping @vrozov 's authorship to unblock this PR's review process by showing @vrozov what was @cloud-fan 's review comment. Actually, it's @cloud-fan 's code in the review comment, but I respected @vrozov 's analysis and finding this bug.

#53176

….sql` SQL golden file ### What changes were proposed in this pull request? This PR aims to clean up `v` properly in `identifier-clause.sql` SQL golden file. Note that this is originated from #53121 with the original authorship although the code is changed [according to the community review comment](#53121 (comment)). ### Why are the changes needed? Due to the lack of a proper clean-up, this causes flaky test failures ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #53121 Closes #53176 from dongjoon-hyun/SPARK-54412-2. Authored-by: Vlad Rozov <vrozov@amazon.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 12bce6b) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

vrozov · 2025-11-24T18:53:16Z

Hi @dongjoon-hyun and @cloud-fan,

Please explain how this DROP VIEW v; is different from DROP VIEW test_view;? The later one causes the same error and it is (unnecessary) clean up code as well.

I think the error in the legacy case does not affect anything, and it is not obvious, is hard to maintain without extra comments or looking into .out files for "legacy" case, or knowing the history of why IF EXIST is there.

cloud-fan · 2025-11-25T01:58:16Z

@vrozov I've explained the reasoning in #53121 (comment) , if you don't agree, we can discuss more but I don't think it worth the time. If you agree but you found some places not following it, feel free to fix them.

[SPARK-54412][TEST] Cleanup (drop) view created in identifier-clause.…

9f12770

…sql golden file

github-actions bot added the SQL label Nov 18, 2025

cloud-fan reviewed Nov 19, 2025

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-54412][TEST] Cleanup (drop) view created in identifier-clause.sql golden file~~ [SPARK-54412][SQL][TEST] Cleanup (drop) view created in identifier-clause.sql golden file Nov 22, 2025

dongjoon-hyun reviewed Nov 22, 2025

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-54412][SQL][TEST] Cleanup (drop) view created in identifier-clause.sql golden file~~ [SPARK-54412][SQL][TEST] Add DROP VIEW v; to identifier-clause* SQL golden files Nov 23, 2025

dongjoon-hyun changed the title ~~[SPARK-54412][SQL][TEST] Add DROP VIEW v; to identifier-clause* SQL golden files~~ [SPARK-54412][SQL][TEST] Clean up v properly in identifier-clause* SQL golden files Nov 23, 2025

dongjoon-hyun changed the title ~~[SPARK-54412][SQL][TEST] Clean up v properly in identifier-clause* SQL golden files~~ [SPARK-54412][SQL][TEST] Clean up v properly in identifier-clause.sql SQL golden file Nov 23, 2025

dongjoon-hyun mentioned this pull request Nov 23, 2025

[SPARK-54412][SQL][TESTS] Clean up v properly in identifier-clause.sql SQL golden file #53176

Closed

dongjoon-hyun reviewed Nov 23, 2025

View reviewed changes

dongjoon-hyun closed this in 12bce6b Nov 23, 2025

[SPARK-54412][SQL][TEST] Clean up v properly in identifier-clause.sql SQL golden file #53121

[SPARK-54412][SQL][TEST] Clean up v properly in identifier-clause.sql SQL golden file #53121

Conversation

vrozov commented Nov 18, 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?

Uh oh!

vrozov commented Nov 18, 2025

Uh oh!

vrozov commented Nov 18, 2025

Uh oh!

cloud-fan Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrozov commented Nov 24, 2025

Uh oh!

cloud-fan commented Nov 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[SPARK-54412][SQL][TEST] Clean up `v` properly in `identifier-clause.sql` SQL golden file #53121

[SPARK-54412][SQL][TEST] Clean up `v` properly in `identifier-clause.sql` SQL golden file #53121

cloud-fan Nov 19, 2025 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading