Skip to content

Fix testFilterWithUdf #17106

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.

Sign up for GitHub

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

Merged
hashhar merged 1 commit into from
Apr 20, 2023
Merged

Fix testFilterWithUdf #17106

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/TestDynamicTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,9 @@ public void testFilterWithPushdownConstraint()
public void testFilterWithUdf()
Copy link
Member

@kokosing kokosing Apr 18, 2023

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

@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! Will do.

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.

Created #17109

Copy link
Member

@kokosing kokosing Apr 18, 2023

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 •
edited
Loading

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.

{
String tableName = realtimeOnlyTable.getTableName();
String query = format("select FlightNum from %s where DivLongestGTimes = FLOOR(EXP(2 * LN(3))) AND 5 < EXP(CarrierDelay) limit 60", tableName.toLowerCase(ENGLISH));
// Note: before Pinot 0.12.1 the below query produced different results due to handling IEEE-754 approximate numerics
// See https://github.com/apache/pinot/issues/10637
String query = format("select FlightNum from %s where DivLongestGTimes = FLOOR(EXP(2 * LN(3)) + 0.1) AND 5 < EXP(CarrierDelay) limit 60", tableName.toLowerCase(ENGLISH));
DynamicTable dynamicTable = buildFromPql(pinotMetadata, new SchemaTableName("default", query), mockClusterInfoFetcher, TESTING_TYPE_CONVERTER);
String expectedPql = "select \"FlightNum\" from realtimeOnly where AND((\"DivLongestGTimes\") = '9.0', (exp(\"CarrierDelay\")) > '5') limit 60";
assertEquals(extractPql(dynamicTable, TupleDomain.all()), expectedPql);
Expand Down