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.

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

Jump to bottom

Fix UUID distinct operation issue #23732

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix UUID distinct operation issue #23732

wants to merge 1 commit into from

Conversation

abhibongale
Copy link

@abhibongale abhibongale commented Sep 26, 2024 •
edited
Loading

  • Addressed UnsupportedOperationException when using GROUP BY with UUIDs.
  • Updated UuidOperators to support distinct operations correctly.
  • Added tests in TestDistinctAggregations to validate the fix.
  • Implemented a fix based on Trino's solution to ensure proper handling of UUID comparison.
  • Cherry picked from commit trinodb/trino#1180 by guyco33

Description

This commit resolves an issue with the UUID type in PrestoDB, where performing a GROUP BY operation on UUIDs resulted in an UnsupportedOperationException. The fix aligns with a solution implemented in Trino, ensuring that UUID comparison works correctly during aggregation.

Motivation and Context

This change is required to allow users to perform GROUP BY operations on UUIDs without encountering errors. It addresses the bug reported in issue #23554, enabling proper usage of UUIDs in queries.

Test Plan

The solution was tested by adding unit tests in TestDistinctAggregations, which cover various scenarios of using distinct operations on UUIDs. All tests passed successfully, confirming the correctness of the implementation.

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.

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Sep 26, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: abhibongale / name: Abhishek Bongale (73b546f)

@tdcmeehan
Copy link
Contributor

Code looks good @abhibongale, thanks for the pull request!

It looks like this is a cherry pick from Trino. Please attribute it in the commit message (see contributing guide).

@abhibongale abhibongale force-pushed the distinct-operation-on-uuid-fails branch from 2abd273 to c20a15c Compare September 26, 2024 19:33
@tdcmeehan
Copy link
Contributor

Almost there. Please use this commit message which will show attribution in Github.

Fix distinct operator for UUID

Cherry-pick of  https://github.com/trinodb/trino/pull/1180 by guyco33
Co-authored-by: Guy Cohen <[email protected]>

aditi-pandit
aditi-pandit reviewed Sep 26, 2024
View reviewed changes
presto-main/src/test/java/com/facebook/presto/sql/query/TestDistinctAggregations.java
@@ -272,4 +272,14 @@ public void testMixedDistinctWithFilter()
"FROM (VALUES (1, 3), (2, 4), (2, 4), (4, 5)) t (x, y)",
"VALUES (BIGINT '0', CAST(NULL AS BIGINT))");
}

@Test
public void testUuidDistinct()
Copy link
Contributor

Choose a reason for hiding this comment

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

@BryanCutler : We should check how this works in Prestissimo as well. Don't anticipate a problem, but it would be good to make it part of the tests.

@abhibongale abhibongale force-pushed the distinct-operation-on-uuid-fails branch from c20a15c to 297f398 Compare September 27, 2024 05:33
imjalpreet
imjalpreet reviewed Sep 27, 2024
View reviewed changes
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

@abhibongale Thanks for the contribution, I have re-triggered your failed test.

A small request, please rebase your branch on master rather than using merge. Merge results in an extra commit as you can see.

@abhibongale
Fix distinct operator for UUID
73b546f 
Cherry-pick of  trinodb/trino#1180 by guyco33
Co-authored-by: Guy Cohen <[email protected]>
@abhibongale abhibongale force-pushed the distinct-operation-on-uuid-fails branch from a3c1a8f to 73b546f Compare September 27, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imjalpreet imjalpreet imjalpreet left review comments

@aditi-pandit aditi-pandit aditi-pandit left review comments

@elharo elharo elharo approved these changes

@jaystarshot jaystarshot Awaiting requested review from jaystarshot jaystarshot is a code owner

@feilong-liu feilong-liu Awaiting requested review from feilong-liu feilong-liu is a code owner

@ClarenceThreepwood ClarenceThreepwood Awaiting requested review from ClarenceThreepwood ClarenceThreepwood is a code owner

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@abhibongale @tdcmeehan @elharo @imjalpreet @aditi-pandit