Skip to content

HIVE-28884: Decouple source.q test from SRC dataset - #5750

Merged
zabetak merged 1 commit into
apache:masterfrom
zabetak:HIVE-28884
Apr 8, 2025
Merged

zabetak merged 1 commit into
apache:masterfrom
zabetak:HIVE-28884

Conversation

@zabetak

@zabetak zabetak commented Apr 7, 2025

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Update the SQL script to simply run two very basic SQL queries that don't require access to any data.

The purpose of the test is to verify that SOURCE CLI command (introduced in HIVE-1415) launches a SQL script but the content of the actual script is not really important.

Why are the changes needed?

  1. Avoid intermittent failures in CI and when the test runs solo.
  2. Speedup test and reduce resource usage.

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

mvn test -Dtest=TestMiniLlapLocalCliDriver -Dqfile=source.q -pl itests/qtest -Pitests

Running source.q test independently fails due to missing SRC table.
The test depends on the presence of the SRC table/dataset but the latter is not declared  explicitly with the qt:dataset command.

The purpose of the test is to verify that SOURCE CLI command (introduced in HIVE-1415) launches a SQL script but the content of the actual script is not really important.

Update the SQL script to simply run two very basic SQL queries that don't require access to any data.
@sonarqubecloud

sonarqubecloud Bot commented Apr 7, 2025

Copy link
Copy Markdown

@soumyakanti3578 soumyakanti3578 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch @zabetak as this was failing intermittently in other builds. I was thinking of creating a patch with --! qt:dataset:src added to source.q, but this is clearly better :)

Comment thread data/files/source.txt

SELECT x.* FROM SRC x;
SELECT 'Executed SQL query 1 using SOURCE command';
SELECT 'Executed SQL query 2 using SOURCE command';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: It probably doesn't matter, but earlier we were testing EXPLAIN too. If you think it is needed, please add an EXPLAIN statement too.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are right. It doesn't matter. As @zabetak wrote:
The goal of the test source.q is to test if the command source works at all.

@InvisibleProgrammer InvisibleProgrammer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for fixing it. I just wasted a couple of hours trying to figure out why this test fails:

  • It failed at my PR
  • And now I understand why it didn't work locally.

The other solution could be to initialize the dataset itself. But your solution is more elegant - as it removes extra, unnecessary statements like actually querying tables.
I just wanted to open a PR that fixes the issue. But with this, I won't do that.

@okumin okumin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1. I'm curious why this test sometimes successfully finished

@zabetak

zabetak commented Apr 8, 2025

Copy link
Copy Markdown
Member Author

@okumin Without the changes the test could finish successfully when it run together (and after) another test that loads the SRC dataset using qt:dataset:src command.

@zabetak
zabetak merged commit ac2c324 into apache:master Apr 8, 2025
@zabetak
zabetak deleted the HIVE-28884 branch April 8, 2025 13:05
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.

6 participants