Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix testFilterWithUdf #17106

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Fix testFilterWithUdf #17106

merged 1 commit into from
Apr 20, 2023

Conversation

elonazoulay
Copy link
Member

This is due to differences in how Pinot handles IEEE-754 approximate numerics pre/post Pinot 0.12.1.

See apache/pinot#10637

Description

Additional context and related issues

Release notes

(X ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

This is due to differences in how Pinot handles IEEE-754
approximate numerics pre/post Pinot 0.12.1.

See apache/pinot#10637
@@ -183,7 +183,9 @@ public void testFilterWithPushdownConstraint()
public void testFilterWithUdf()
Copy link
Member

Choose a reason for hiding this comment

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

Can you please fix pinot tests to use capital letters for SQL keywords like: SELECT, FROM, WHERE? Let's do it as separate Pr.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! Will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #17109

@@ -183,7 +183,9 @@ public void testFilterWithPushdownConstraint()
public void testFilterWithUdf()
Copy link
Member

Choose a reason for hiding this comment

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

Can you please let me know what are you fixing here? Is this a flaky issue? Do you have any github issues reported for the problem you are fixing? How the problem you are fixing manifests?

Copy link
Member Author

@elonazoulay elonazoulay Apr 18, 2023

Choose a reason for hiding this comment

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

Sure, the issue is created for Pinot: apache/pinot#10637.
Before Pinot 0.12.1 the Pinot expression FLOOR(EXP(2 * LN(3))) return 9.0 and after 0.12.1 returns 8.0 - more details in the Pinot issue.

It is not a flaky test, it is due to differences in how Pinot handles IEEE-754 approximate numerics - specific info which affected this test is in the pinot issue.

The behavior is deterministic: pre 0.12.1 and post 0.12.1 the results are the same, just different across Pinot versions.

The problem manifested with a failing TestDynamicTable::testFilterWithUdf. The test passes with Pinot 11.0 libraries. Also running 11.0 Pinot and 12.1 Pinot and issuing the query shows different results - the specific results are in the pinot issue.

lmk if you need any more info.

@ebyhr ebyhr removed their request for review April 18, 2023 21:57
@elonazoulay elonazoulay requested a review from kokosing April 18, 2023 23:33
@hashhar
Copy link
Member

hashhar commented Apr 19, 2023

before merging this we need to verify if the Pinot results actually depend on JVM version being used instead of the Pinot version itself.

@elonazoulay
Copy link
Member Author

elonazoulay commented Apr 20, 2023

before merging this we need to verify if the Pinot results actually depend on JVM version being used instead of the Pinot version itself.

I tried this on java11 and java17 and they behave the same. Changing Pinot version results in the different behavior. Also updated the pinot issue with those findings.

To repro:
Run the TestDynamicTable::testFilterWithUdf pre and post the commit that upgrades trino-pinot libraries to 0.12.1 - this uses java17.

Run the "latest" and "previous" version from the base connector smoke test, do docker ps --no-trunc | grep StartController and open the query console from the port mapped to Pinot 9000 (the controller and ui port) and issue a query like this:

SELECT <column>, exp(2 * ln(3)) from <table>

@elonazoulay
Copy link
Member Author

Also, adding .1 to the query in the test guarantees that the actual and expected result will be equal regardless of the version.

lmk if you need more info.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Indeed, I can see the different behaviour as well. Thanks for including repro steps.

LGTM.

@hashhar hashhar merged commit 6cb5e7f into trinodb:master Apr 20, 2023
@github-actions github-actions bot added this to the 415 milestone Apr 20, 2023
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.

4 participants