Skip to content

Make Iceberg Dynamic Filter test more resilient#10997

Merged
findepi merged 1 commit intotrinodb:masterfrom
alexjo2144:iceberg/fix-flaky-test
Feb 18, 2022
Merged

Make Iceberg Dynamic Filter test more resilient#10997
findepi merged 1 commit intotrinodb:masterfrom
alexjo2144:iceberg/fix-flaky-test

Conversation

@alexjo2144
Copy link
Copy Markdown
Member

Description

Fixes: #10932

General information

Is this change a fix, improvement, new feature, refactoring, or other?

Flaky test fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Test only

How would you describe this change to a non-technical end user or system administrator?

n/a

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Feb 9, 2022
@alexjo2144
Copy link
Copy Markdown
Member Author

I'll remove the invocationCount before merge, just wanted to test it a few times on CI

@alexjo2144 alexjo2144 requested review from findepi and hashhar February 9, 2022 21:56
@alexjo2144 alexjo2144 added the WIP label Feb 9, 2022
@findepi findepi requested a review from sopel39 February 10, 2022 13:04
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.

(reminder)

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.

what is [4]?

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 guess those are stats which correspond to totalprice

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, 4 is the fieldId for totalPrice. I looked through the $ metadata tables to see if there was a way to query for that value but couldn't find a good way of doing it

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.

// should find such a price

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.

But why are we sure gonna find it?
If we have two files with non overlaping ranges we get empty result range.

E.g:

  • f1: [10, 20]
  • f2: [3,7]

We get range from the first query: [10, 7]. Which yields empty result from second query.

I am probably missing sth.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That is possible, and would break the test, but seems unlikely. The overlapping range when I ran the test was most of the range of the column. Unless the data is explicitly ordered I think there should be some overlap.

If this still feels too flaky we can take it out and try testing the DF a different way.

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.

why removed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is guaranteed by the query now, and driver count stats are kinda flaky.

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 had same issue when adding MinIO test's for Iceberg. (this assertion was removed)
Would be nice to merge this PR earlier: #10894 (comment)
than https://github.com/trinodb/trino/pull/10894/checks

@hashhar hashhar removed their request for review February 10, 2022 13:21
Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka left a comment

Choose a reason for hiding this comment

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

lgtm % @findepi comments

@alexjo2144 alexjo2144 force-pushed the iceberg/fix-flaky-test branch from 7e24b12 to 0b127ec Compare February 17, 2022 15:58
@alexjo2144 alexjo2144 removed the WIP label Feb 17, 2022
@alexjo2144
Copy link
Copy Markdown
Member Author

I removed the invocationCount used for testing, and added another code comment about the assumption of there being an overlapping range in all files. @findepi @losipiuk

@findepi findepi merged commit dd92d44 into trinodb:master Feb 18, 2022
@findepi findepi added the test label Feb 18, 2022
@alexjo2144 alexjo2144 deleted the iceberg/fix-flaky-test branch February 18, 2022 15:23
@github-actions github-actions bot added this to the 372 milestone Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Flaky TestIcebergParquetConnectorTest.testLocalDynamicFilteringWithSelectiveBuildSizeJoin

5 participants