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

Tighten checks for "bins" elements in width_bucket(x, bins) #24103

Merged

Conversation

spershin
Copy link
Contributor

@spershin spershin commented Nov 20, 2024

Description

Make width_bucket(x, bins) throw error if it finds a null or non-finite element in bins.

Motivation and Context

Solves #24055

Impact

Changes behavior of width_bucket(x, bins) which previously was treating all null elements in bins as zero. Now the function will throw an error.

Test Plan

Updated unit test to handle few cases with nulls.

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

== RELEASE NOTES ==
General Changes
* Fix behavior of :func:`width_bucket(x, bins) -> bigint` which previously treated all ``null`` elements in bins as ``0``. Now the function will throw an error if it finds a ``null`` or non-finite element in ``bins``..  :pr:`24103`

@spershin spershin requested a review from rschlussel November 20, 2024 19:27
@spershin spershin requested review from steveburnett, elharo and a team as code owners November 20, 2024 19:27
@spershin spershin requested a review from presto-oss November 20, 2024 19:27
@spershin spershin force-pushed the CheckNullElementsInWidthBucketFunc branch from db34c17 to e27c342 Compare November 21, 2024 05:03
@spershin spershin changed the title Make width_bucket(x, bins) throw error if found a null element in "bins" Tighten checks for "bins" elements in width_bucket(x, bins) Nov 21, 2024
@spershin spershin requested a review from rschlussel November 21, 2024 05:05
rschlussel
rschlussel previously approved these changes Nov 21, 2024
Copy link
Contributor

@rschlussel rschlussel left a comment

Choose a reason for hiding this comment

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

I'd undo the * 1 change for clarity, but everything else looks good

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the doc! I made a few suggestions, but thought it would be easier to show the result of my suggestions with a local edit, local doc build, and a screenshot.
Screenshot 2024-11-21 at 11 31 26 AM

presto-docs/src/main/sphinx/functions/math.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/functions/math.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/functions/math.rst Outdated Show resolved Hide resolved
@steveburnett
Copy link
Contributor

Because this PR changes user-facing behavior, I think we should consider a release note entry for this PR. Perhaps something like:

== RELEASE NOTES ==
General Changes
* Fix behavior of :func:`width_bucket(x, bins) -> bigint` which previously treated all ``null`` elements in bins as ``0``. Now the function will throw an error.  :pr:`24103`

(I did a local doc build to verify that :func:width_bucket(x, bins) -> bigint generates a working link from a file in /release/ to this function in functions/math.rst.)

@tdcmeehan tdcmeehan added the from:Meta PR from Meta label Nov 22, 2024
@prestodb-ci
Copy link

Saved that user @spershin is from Meta

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull updated branch, new local docs build, looks good. Thank you for the quick revision!

@steveburnett
Copy link
Contributor

steveburnett commented Nov 22, 2024

Nit on the formatting of the release note entry - please add a row of three ` above and below the release note block. Like this:

== RELEASE NOTES ==

General Changes
* Fix behavior of :func:`width_bucket(x, bins) -> bigint` which previously treated all ``null`` elements in bins as ``0``. Now the function will throw an error if it finds a ``null`` or non-finite element in ``bins``..  :pr:`24103`

@spershin spershin merged commit 72e5c57 into prestodb:master Nov 22, 2024
58 checks passed
spershin pushed a commit to spershin/velox-1 that referenced this pull request Nov 22, 2024
Summary:
Make width_bucket(x, bins) throw error if it finds a null or
non-finite element in bins.
As per new Presto Java behavior: prestodb/presto#24103

Differential Revision: D66382264
spershin pushed a commit to spershin/velox-1 that referenced this pull request Nov 22, 2024
…incubator#11629)

Summary:

Make width_bucket(x, bins) throw error if it finds a null or
non-finite element in bins.
As per new Presto Java behavior: prestodb/presto#24103

Reviewed By: Yuhta

Differential Revision: D66382264
spershin pushed a commit to spershin/velox-1 that referenced this pull request Nov 22, 2024
…incubator#11629)

Summary:

Make width_bucket(x, bins) throw error if it finds a null or
non-finite element in bins.
As per new Presto Java behavior: prestodb/presto#24103

Reviewed By: Yuhta

Differential Revision: D66382264
facebook-github-bot pushed a commit to facebookincubator/velox that referenced this pull request Nov 23, 2024
Summary:
Pull Request resolved: #11629

Make width_bucket(x, bins) throw error if it finds a null or
non-finite element in bins.
As per new Presto Java behavior: prestodb/presto#24103

Reviewed By: Yuhta

Differential Revision: D66382264

fbshipit-source-id: 84f549dfa313e1794551fba9d71f8b87eb3b713e
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
…incubator#11629)

Summary:
Pull Request resolved: facebookincubator#11629

Make width_bucket(x, bins) throw error if it finds a null or
non-finite element in bins.
As per new Presto Java behavior: prestodb/presto#24103

Reviewed By: Yuhta

Differential Revision: D66382264

fbshipit-source-id: 84f549dfa313e1794551fba9d71f8b87eb3b713e
@unidevel unidevel mentioned this pull request Jan 28, 2025
26 tasks
@unidevel unidevel mentioned this pull request Jan 28, 2025
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:Meta PR from Meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants