Skip to content

[Native] Add approx_distinct e2e tests for decimal type#21464

Merged
aditi-pandit merged 1 commit intoprestodb:masterfrom
karteekmurthys:native-app-distinct-decimal
Dec 16, 2023
Merged

[Native] Add approx_distinct e2e tests for decimal type#21464
aditi-pandit merged 1 commit intoprestodb:masterfrom
karteekmurthys:native-app-distinct-decimal

Conversation

@karteekmurthys
Copy link
Contributor

@karteekmurthys karteekmurthys commented Nov 30, 2023

Description

Add approx_distinct e2e tests for decimals.

Motivation and Context

Required for running analyze command on tables with decimal columns.

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

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ...
* ...

Hive Changes
* ...
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

@karteekmurthys karteekmurthys requested review from a team as code owners November 30, 2023 22:22
@karteekmurthys karteekmurthys self-assigned this Nov 30, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 30, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: karteekmurthys / name: Karteek (493431f)

aditi-pandit
aditi-pandit reviewed Nov 30, 2023
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @karteekmurthys. Took a quick look.

@karteekmurthys karteekmurthys force-pushed the native-app-distinct-decimal branch 2 times, most recently from a2b0e18 to 1d22672 Compare November 30, 2023 23:00
@karteekmurthys karteekmurthys requested review from a team and aditi-pandit November 30, 2023 23:08
@karteekmurthys karteekmurthys mentioned this pull request Dec 5, 2023
Merged
@karteekmurthys karteekmurthys force-pushed the native-app-distinct-decimal branch from 1d22672 to 6d8badc Compare December 7, 2023 22:19
@karteekmurthys karteekmurthys changed the title [Native] Add approx_distinct e2e tests for decimal type [Native] Advance velox and add approx_distinct e2e tests for decimal type Dec 7, 2023
@karteekmurthys karteekmurthys force-pushed the native-app-distinct-decimal branch 4 times, most recently from 27c86f0 to b84e348 Compare December 13, 2023 20:24
@karteekmurthys karteekmurthys changed the title [Native] Advance velox and add approx_distinct e2e tests for decimal type [Native] Add approx_distinct e2e tests for decimal type Dec 14, 2023
@karteekmurthys karteekmurthys force-pushed the native-app-distinct-decimal branch from b84e348 to 7dd0697 Compare December 14, 2023 21:29
aditi-pandit
aditi-pandit reviewed Dec 15, 2023
View reviewed changes
...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java Outdated
Copy link
Contributor

@aditi-pandit aditi-pandit Dec 15, 2023

Choose a reason for hiding this comment

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

Split this and above logic into individual methods.

@aditi-pandit
Copy link
Contributor

aditi-pandit commented Dec 15, 2023

@karteekmurthys : Can you remove the Velox submodule changes from this PR ? They are not needed anymore.

@karteekmurthys karteekmurthys force-pushed the native-app-distinct-decimal branch 2 times, most recently from e2a5255 to 378c893 Compare December 15, 2023 06:17
aditi-pandit
aditi-pandit reviewed Dec 15, 2023
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @karteekmurthys. Minor nit.

...ecution/src/test/java/com/facebook/presto/nativeworker/AbstractTestNativeGeneralQueries.java Outdated
Copy link
Contributor

@aditi-pandit aditi-pandit Dec 15, 2023

Choose a reason for hiding this comment

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

Nit : Remove this line. The test has both Short and Long decimals.

Copy link
Contributor Author

@karteekmurthys karteekmurthys Dec 15, 2023

Choose a reason for hiding this comment

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

Done.

@karteekmurthys karteekmurthys force-pushed the native-app-distinct-decimal branch from 378c893 to 493431f Compare December 15, 2023 23:34
aditi-pandit
aditi-pandit approved these changes Dec 16, 2023
View reviewed changes
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @karteekmurthys

@aditi-pandit aditi-pandit merged commit e7a29fc into prestodb:master Dec 16, 2023
@amitkdutta amitkdutta mentioned this pull request Dec 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aditi-pandit aditi-pandit aditi-pandit approved these changes

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

Assignees

@karteekmurthys karteekmurthys

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@karteekmurthys @aditi-pandit