[native] Add floating point aggregate tests with NaN #21447

czentgr · 2023-11-28T04:27:32Z

Description

This PR is adding E2E tests to test floating point NaN values in the various structs and to ensure the RowContainer in Velox works properly.

Motivation and Context

There was and still is an issue handling NaN values in Velox due to the fact NaN is a special value and the C++ behavior does not match the desired database behavior.

Impact

N/A

Test Plan

Adding new tests.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== NO RELEASE NOTE ==

aditi-pandit · 2023-11-28T19:56:21Z

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java

+        assertQueryError("SELECT DISTINCT a/a FROM (VALUES (0.0e0), (0.0e0)) x (a)", notEqualRowsErrorMsg);
+        assertQueryError("SELECT * FROM (VALUES nan(), nan(), nan()) GROUP BY 1", notEqualRowsErrorMsg);
+        assertQueryError("SELECT a, b, c FROM (VALUES ROW(nan(), 1, 2), ROW(nan(), 1, 2)) t(a, b, c) GROUP BY 1, 2, 3", notEqualRowsErrorMsg);
+        assertQuery("SELECT a FROM (VALUES (ARRAY[nan(), 2e0, 3e0]), (ARRAY[nan(), 2e0, 3e0])) t(a) GROUP BY a");


You might want to move this query ahead as its the only one related to the previous fix you made.

aditi-pandit · 2023-11-28T19:59:01Z

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java

+    {
+        // Fix Velox to get these tests passed.
+        String notEqualRowsErrorMsg = "*.not equal.*";
+        assertQueryError("SELECT DISTINCT a/a FROM (VALUES (0.0e0), (0.0e0)) x (a)", notEqualRowsErrorMsg);


Maybe also try an aggregation requiring Sorted inputs like say
SELECT a, array_agg(a ORDER BY a) FROM (VALUES (0.0e0), (0.0e0)) x (a) GROUP BY 1

Added this along with NaN values in the table as well.
This also fails as expected.

aditi-pandit · 2023-11-28T21:07:31Z

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java

+        String notEqualRowsErrorMsg = "*.not equal.*";
+        assertQueryError("SELECT DISTINCT a/a FROM (VALUES (0.0e0), (0.0e0)) x (a)", notEqualRowsErrorMsg);
+        assertQueryError("SELECT * FROM (VALUES nan(), nan(), nan()) GROUP BY 1", notEqualRowsErrorMsg);
+        assertQueryError("SELECT a, b, c FROM (VALUES ROW(nan(), 1, 2), ROW(nan(), 1, 2)) t(a, b, c) GROUP BY 1, 2, 3", notEqualRowsErrorMsg);


Its interesting this didn't work either. Is this because of GROUP BY 1, 2, 3 ? If we do just GROUP BY 1 does this work ?

The values clause is turned into a three column table of normalized (simple) values. So you get the failure in the equals for the NaN.

When changed to GROUP BY 1 the query fails with

Runtime line 1:11: 'b' must be an aggregate expression or appear in GROUP BY clause

because the other columns in the projection have no aggregation defined either via group by, or sum etc.
Running

SELECT a, SUM(b), SUM(c) FROM (VALUES ROW(nan(), 1, 2), ROW(nan(), 1, 2)) t(a, b, c) GROUP BY 1

results in the error as expected.

I tried various things to have the ROW as values in a single column but it somehow always either results in two columns or three columns.

Technically, this should have worked based on the syntax.

SELECT a FROM (VALUES (ROW(nan(), 1, 2)), (ROW(nan(), 1, 2)) ) t(a) GROUP BY 1

Maybe I'm missing something. This complains that the table has 3 columns.

@czentgr : I see. I expected this to be a single column with ROW types as well. I guess right now the ROW usage is redundant.

aditi-pandit · 2023-11-28T21:09:10Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueryFramework.java

+
+    protected void assertQueryError(QueryRunner queryRunner, Session session, @Language("SQL") String sql, @Language("RegExp") String expectedMessageRegExp)
+    {
+        try {


Why do you have the try block here ?

Does assertQueryError(queryRunner, session, sql, expectedMessageRegExp); not work ?

This is a new type of function that specifically checks if the AssertionError is thrown by the QueryRunner. This indicates a failure on the JUnit test assertions. There was no such function in the QueryAssertions class and I opted to add it here.
I'm going to swap the definitions so it is easier to see.

The function without the session argument is a helper which calls this function.

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java

czentgr

Thank you for the review!

czentgr · 2023-11-28T22:38:22Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueryFramework.java

+
+    protected void assertQueryError(QueryRunner queryRunner, Session session, @Language("SQL") String sql, @Language("RegExp") String expectedMessageRegExp)
+    {
+        try {


This is a new type of function that specifically checks if the AssertionError is thrown by the QueryRunner. This indicates a failure on the JUnit test assertions. There was no such function in the QueryAssertions class and I opted to add it here.
I'm going to swap the definitions so it is easier to see.

The function without the session argument is a helper which calls this function.

czentgr · 2023-11-29T00:31:20Z

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java

+        String notEqualRowsErrorMsg = "*.not equal.*";
+        assertQueryError("SELECT DISTINCT a/a FROM (VALUES (0.0e0), (0.0e0)) x (a)", notEqualRowsErrorMsg);
+        assertQueryError("SELECT * FROM (VALUES nan(), nan(), nan()) GROUP BY 1", notEqualRowsErrorMsg);
+        assertQueryError("SELECT a, b, c FROM (VALUES ROW(nan(), 1, 2), ROW(nan(), 1, 2)) t(a, b, c) GROUP BY 1, 2, 3", notEqualRowsErrorMsg);


The values clause is turned into a three column table of normalized (simple) values. So you get the failure in the equals for the NaN.

When changed to GROUP BY 1 the query fails with

Runtime line 1:11: 'b' must be an aggregate expression or appear in GROUP BY clause

because the other columns in the projection have no aggregation defined either via group by, or sum etc.
Running

SELECT a, SUM(b), SUM(c) FROM (VALUES ROW(nan(), 1, 2), ROW(nan(), 1, 2)) t(a, b, c) GROUP BY 1

results in the error as expected.

I tried various things to have the ROW as values in a single column but it somehow always either results in two columns or three columns.

Technically, this should have worked based on the syntax.

SELECT a FROM (VALUES (ROW(nan(), 1, 2)), (ROW(nan(), 1, 2)) ) t(a) GROUP BY 1

Maybe I'm missing something. This complains that the table has 3 columns.

czentgr · 2023-11-29T00:40:48Z

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java

+    {
+        // Fix Velox to get these tests passed.
+        String notEqualRowsErrorMsg = "*.not equal.*";
+        assertQueryError("SELECT DISTINCT a/a FROM (VALUES (0.0e0), (0.0e0)) x (a)", notEqualRowsErrorMsg);


Added this along with NaN values in the table as well.
This also fails as expected.

yzhang1991 · 2024-05-02T06:45:46Z

@czentgr can we revisit this since facebookincubator/velox#7780 is merged?
cc @aditi-pandit

czentgr · 2024-05-03T18:44:42Z

This tests various NaN cases which do not all work. The commit only actually fixed one of them.
We can add this with the queryAssertError for the failing queries if we agree.

The outstanding is that Java and Prestissimo/Velox have different behavior and even the Java behavior is not consistent. There is work to fix this and make it consistent.

yzhang1991 · 2024-05-06T15:22:08Z

@aditi-pandit need your input on this, thx!

aditi-pandit

Thanks @czentgr.

I'm fine to add these tests with queryAssertError.

aditi-pandit · 2024-05-09T20:41:14Z

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java

+        String notEqualRowsErrorMsg = "*.not equal.*";
+        assertQueryError("SELECT DISTINCT a/a FROM (VALUES (0.0e0), (0.0e0)) x (a)", notEqualRowsErrorMsg);
+        assertQueryError("SELECT * FROM (VALUES nan(), nan(), nan()) GROUP BY 1", notEqualRowsErrorMsg);
+        assertQueryError("SELECT a, b, c FROM (VALUES ROW(nan(), 1, 2), ROW(nan(), 1, 2)) t(a, b, c) GROUP BY 1, 2, 3", notEqualRowsErrorMsg);


@czentgr : I see. I expected this to be a single column with ROW types as well. I guess right now the ROW usage is redundant.

aditi-pandit · 2024-05-09T20:49:38Z

@czentgr : Please can you rebase your code.

I can merge these tests.

aditi-pandit

Thanks @czentgr

aditi-pandit · 2024-05-14T20:22:54Z

@tdcmeehan, @yingsu00 : Please can you help review the changes in AbstractTestQueryFramework.

yingsu00

@czentgr Looks good, pending the more detailed expected error message in the comment.

yingsu00 · 2024-05-22T22:17:04Z

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java

+    {
+        // Fix Velox to get these tests passed.
+        // See https://github.com/prestodb/presto/issues/20283
+        String notEqualRowsErrorMsg = "*.not equal.*";


just one nit: will you be able to paste the longer version of the expected error message in the comment?

czentgr marked this pull request as ready for review November 28, 2023 15:07

czentgr requested review from a team as code owners November 28, 2023 15:07

czentgr requested review from presto-oss, aditi-pandit and mknegi November 28, 2023 15:07

aditi-pandit reviewed Nov 28, 2023

View reviewed changes

aditi-pandit changed the title ~~[native] Add floating point aggregate NaN tests~~ [native] Add floating point aggregate tests with NaN Nov 28, 2023

aditi-pandit reviewed Nov 28, 2023

View reviewed changes

...execution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeAggregations.java Show resolved Hide resolved

czentgr commented Nov 29, 2023

View reviewed changes

czentgr force-pushed the cz_nan_e2e_test branch from f8fa95a to c899829 Compare November 29, 2023 03:53

mknegi previously approved these changes Nov 29, 2023

View reviewed changes

aditi-pandit previously approved these changes May 9, 2024

View reviewed changes

[native] Add floating point aggregate tests with NaN

6c45c05

czentgr dismissed stale reviews from aditi-pandit and mknegi via 6c45c05 May 14, 2024 18:31

czentgr force-pushed the cz_nan_e2e_test branch from c899829 to 6c45c05 Compare May 14, 2024 18:31

czentgr requested review from jaystarshot and feilong-liu as code owners May 14, 2024 18:31

aditi-pandit approved these changes May 14, 2024

View reviewed changes

yingsu00 approved these changes May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native] Add floating point aggregate tests with NaN #21447

[native] Add floating point aggregate tests with NaN #21447

czentgr commented Nov 28, 2023 •

edited

aditi-pandit Nov 28, 2023

aditi-pandit Nov 28, 2023

czentgr Nov 29, 2023

aditi-pandit Nov 28, 2023

czentgr Nov 29, 2023

aditi-pandit May 9, 2024

aditi-pandit Nov 28, 2023

czentgr Nov 28, 2023

czentgr left a comment

czentgr Nov 28, 2023

czentgr Nov 29, 2023

czentgr Nov 29, 2023

yzhang1991 commented May 2, 2024

czentgr commented May 3, 2024

yzhang1991 commented May 6, 2024

aditi-pandit left a comment •

edited

aditi-pandit May 9, 2024

aditi-pandit commented May 9, 2024

aditi-pandit left a comment

aditi-pandit commented May 14, 2024

yingsu00 left a comment

yingsu00 May 22, 2024

[native] Add floating point aggregate tests with NaN #21447

Are you sure you want to change the base?

[native] Add floating point aggregate tests with NaN #21447

Conversation

czentgr commented Nov 28, 2023 • edited

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czentgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yzhang1991 commented May 2, 2024

czentgr commented May 3, 2024

yzhang1991 commented May 6, 2024

aditi-pandit left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-pandit commented May 9, 2024

aditi-pandit left a comment

Choose a reason for hiding this comment

aditi-pandit commented May 14, 2024

yingsu00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czentgr commented Nov 28, 2023 •

edited

aditi-pandit left a comment •

edited