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

fix getNumberOfArguments() for s2Rect functions #28663

Merged
merged 1 commit into from
Sep 7, 2021
Merged

fix getNumberOfArguments() for s2Rect functions #28663

merged 1 commit into from
Sep 7, 2021

Conversation

bharatnc
Copy link
Contributor

@bharatnc bharatnc commented Sep 6, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Bug Fix (user-visible misbehaviour in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

  • Fix the number of arguments required by s2RectAdd and s2RectContains functions.

Detailed description / Documentation draft:

This fixes the value returned by the getNumberOfArguments() by the
s2RectAdd and the s2RectContains functions. Only 3 arguments are
used by these functions and not 4:

  • low s2Point of rectangle
  • high s2Point of rectangle
  • given s2Point

The given s2Point is used to groow the size of the bounding rectangle to
include the given S2Point in case of the s2RectAdd function. In case
of s2RectContains, the function determines if the bounded rectangle
contains the given point.

PS: I wonder if it's more apt to call rectAdd as rectGrow since
it's used to grow the size of a given rectangle.

@robot-clickhouse robot-clickhouse added the pr-bugfix Pull request with bugfix, not backported by default label Sep 6, 2021
@bharatnc
Copy link
Contributor Author

bharatnc commented Sep 6, 2021

cc: @nikitamikhaylov

@nikitamikhaylov
Copy link
Member

Thanks @bharatnc !

@nikitamikhaylov nikitamikhaylov self-assigned this Sep 6, 2021
@bharatnc
Copy link
Contributor Author

bharatnc commented Sep 6, 2021

I'll see if I can open another PR later to add some tests for the s2Rect functions.

This fixes the value returned by the getNumberOfArguments() by the
s2RectAdd and the s2RectContains functions. Only 3 arguments are
used by these functions and not 4:

- low s2Point of rectangle
- high s2Point of rectangle
- given s2Point

The given s2Point is used to groow the size of the bounding rectangle to
include the given S2Point in case of the s2RectAdd function. In case
of s2RectContains, the function determines if the bounded rectangle
contains the given point.

PS: I wonder if it's more apt to call rectAdd as rectGrow since
it's used to grow the size of a given rectangle.
@nikitamikhaylov nikitamikhaylov merged commit ff59105 into ClickHouse:master Sep 7, 2021
alexey-milovidov added a commit that referenced this pull request Sep 8, 2021
Backport #28663 to 21.9: fix getNumberOfArguments() for s2Rect functions
nikitamikhaylov added a commit that referenced this pull request Sep 10, 2021
Backport #28663 to 21.10: fix getNumberOfArguments() for s2Rect functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants