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

….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 apache#53121 with the original authorship although the code is changed [according to the community review comment](apache#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 apache#53121 Closes apache#53176 from dongjoon-hyun/SPARK-54412-2. Authored-by: Vlad Rozov <vrozov@amazon.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

vrozov · 2025-11-25T16:22:41Z

@cloud-fan I don't agree with your reasoning, as I already mentioned several times in my comments: #53121 (comment), #53121 (comment). Somehow, it was important for you to discuss this change, and

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

Line 321 in d14209c

DROP VIEW test_view;

does not matter.

Consider identifier-clause.sql (not a legacy case). Will you use IF EXIST for the view that is created in the same script?

Anyway, if it is important for you and the community to have less "noise", I would expect other similar cases where a table or view fails to be created and then dropped to be updated to use IF EXIST at least to be consistent and avoid confusion from other committers.

cloud-fan · 2025-11-26T06:50:03Z

Will you use IF EXIST for the view that is created in the same script?

Yes if the view is not guaranteed to be created. @vrozov we need to understand that reality does not always match our expectation. You are right about the reality, but what I said before is about expectation. I'll be surprised if you don't agree with the expectation (less noise), and if you agree, you are welcome to improve the reality to better match the expectation.

vrozov · 2025-12-02T17:28:27Z

@cloud-fan

Yes if the view is not guaranteed to be created.

In the case of identifier-clause.sql, the view is guaranteed to be created. If it is not created, the test would fail.

I'll be surprised if you don't agree with the expectation

Expectations are

View is created, select succeeds, and drop succeeds in case of identifier-clause.sql
View is not created, select fails, and drop fails in case of identifier-clause-legacy.sql
Optionally, you can use IF EXIST clause, but in that case, it should be consistent across all usages of DROP clause.

I do agree that "less noise" is preferred (for example large number of warnings during compilation), but I don't agree that it applies here. Errors and warnings are not always "noise". Golden files are designed to handle both positive and negative cases, and in the case of identifier-clause-legacy.sql, it is simply the negative case, not "noise".

cloud-fan · 2025-12-02T21:27:48Z

the view is guaranteed to be created

This is not correct. The same golden file can be tested with different setups, so the view may not be created if legacy IDENTIFY is enabled.

vrozov · 2025-12-03T02:33:50Z

Assume for a second that the legacy case does not exist. Consider a case where someone creates a brand new file that creates a view, selects from the view, and drops the view. Will you use IF EXIST in that case? After some time, another developer may use that file in a different setup where the create view fails. Will you ask that developer to change the first file?

[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

Uh oh!

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!

vrozov commented Nov 25, 2025

Uh oh!

cloud-fan commented Nov 26, 2025

Uh oh!

vrozov commented Dec 2, 2025

Uh oh!

cloud-fan commented Dec 2, 2025

Uh oh!

vrozov commented Dec 3, 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