Skip to content

Ensure test table contains multiple active files#13902

Merged
findepi merged 2 commits intotrinodb:masterfrom
findinpath:fix-flaky-table-statistics-tests
Sep 9, 2022
Merged

Ensure test table contains multiple active files#13902
findepi merged 2 commits intotrinodb:masterfrom
findinpath:fix-flaky-table-statistics-tests

Conversation

@findinpath
Copy link
Copy Markdown
Contributor

Description

Ensure that the table created in the test does indeed have multiple files by partitioning the table.
Assuming that an unpartitioned table will have multiple files after being created with a CTAS statement can lead to flakiness in the tests.

This change is related to test infrastructure and doesn't change the existing functionality of the production code.

Related issues, pull requests, and links

Fixes #13107

@cla-bot cla-bot bot added the cla-signed label Aug 29, 2022
@findinpath findinpath added the no-release-notes This pull request does not require release notes entry label Aug 29, 2022
@alexjo2144
Copy link
Copy Markdown
Member

There's already one test with a partitioned table. Can we instead do a CTAS + INSERT to force multiple files?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume UNION (should be UNION ALL, really) was used to ensure multiple data files.

should we drop the second UNION branch now?

@findinpath findinpath force-pushed the fix-flaky-table-statistics-tests branch from 93dc211 to ad623fe Compare August 30, 2022 15:29
@findinpath
Copy link
Copy Markdown
Contributor Author

Migrated the tests to use a CREATE TABLE statement and INSERT instead of CTAS as before.

@findinpath findinpath force-pushed the fix-flaky-table-statistics-tests branch from ad623fe to 5ae0f8b Compare August 30, 2022 15:30
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i must admit i don't know what testMultiFileTable() is for.
Maybe we can remove it, or add a javadoc explaining its value
cc @alexjo2144

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I may have known at one point, but I'm also unsure if it has any value

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if we don't know why we have a test, we don't know what's a good change and what isn't for the test prep or assertions.
let's drop it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I dropped the useless test in a separate PR.
testMultiFileTableWithNaNValue has been reworked to ensure that the table tested contains multiple active files.

@findinpath findinpath force-pushed the fix-flaky-table-statistics-tests branch from 5ae0f8b to 0bcc4ba Compare August 31, 2022 05:01
@findinpath findinpath force-pushed the fix-flaky-table-statistics-tests branch from 0bcc4ba to e24f30b Compare August 31, 2022 11:34
@findinpath findinpath requested a review from findepi September 8, 2022 14:57
@findepi findepi merged commit 0a5d02a into trinodb:master Sep 9, 2022
@github-actions github-actions bot added this to the 396 milestone Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

Flaky TestDeltaLakeCreateTableStatistics.testMultiFileTable

3 participants