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

Auto-generate aggregation classes #1918

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

miguelgrinberg
Copy link
Collaborator

@miguelgrinberg miguelgrinberg commented Sep 13, 2024

This change adds docstrings and types to all aggregation classes.

@miguelgrinberg miguelgrinberg force-pushed the generate-aggregations branch 5 times, most recently from 0d6d89d to 4212e69 Compare September 27, 2024 09:48
@miguelgrinberg miguelgrinberg marked this pull request as ready for review September 27, 2024 10:08
@@ -220,7 +220,13 @@ def test_filters_correctly_identifies_the_hash() -> None:


def test_bucket_sort_agg() -> None:
bucket_sort_agg = aggs.BucketSort(sort=[{"total_sales": {"order": "desc"}}], size=3)
bucket_sort_agg = aggs.BucketSort(sort=[{"total_sales": {"order": "desc"}}], size=3) # type: ignore
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For several examples in this file I kept the original dict based solution with ignored typing errors, and right below I've added a correctly typed version, so that we make sure we are backwards compatible.

Copy link
Member

Choose a reason for hiding this comment

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

This deserves to be an in-file comment IMO, because it's tempting to remove in a refactoring.

return FieldBucketData(self, search, data)


class RandomSampler(Bucket[_R]):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm manually adding the missing RandomSampler for now. I think once it is added to the schema this is going to cause a failure in the type checking, so I'll know I can remove it then.

Copy link
Member

Choose a reason for hiding this comment

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

RandomSampler is in the specification now. I've just backported to 8.15 in case it helps here.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM.

@@ -220,7 +220,13 @@ def test_filters_correctly_identifies_the_hash() -> None:


def test_bucket_sort_agg() -> None:
bucket_sort_agg = aggs.BucketSort(sort=[{"total_sales": {"order": "desc"}}], size=3)
bucket_sort_agg = aggs.BucketSort(sort=[{"total_sales": {"order": "desc"}}], size=3) # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

This deserves to be an in-file comment IMO, because it's tempting to remove in a refactoring.

Comment on lines -879 to +1695
query: Union[str, float, bool, DefaultType] = DEFAULT,
analyzer: Union[str, DefaultType] = DEFAULT,
auto_generate_synonyms_phrase_query: Union[bool, DefaultType] = DEFAULT,
cutoff_frequency: Union[float, DefaultType] = DEFAULT,
fuzziness: Union[str, int, DefaultType] = DEFAULT,
fuzzy_rewrite: Union[str, DefaultType] = DEFAULT,
fuzzy_transpositions: Union[bool, DefaultType] = DEFAULT,
lenient: Union[bool, DefaultType] = DEFAULT,
max_expansions: Union[int, DefaultType] = DEFAULT,
minimum_should_match: Union[int, str, DefaultType] = DEFAULT,
operator: Union[Literal["and", "or"], DefaultType] = DEFAULT,
prefix_length: Union[int, DefaultType] = DEFAULT,
zero_terms_query: Union[Literal["all", "none"], DefaultType] = DEFAULT,
boost: Union[float, DefaultType] = DEFAULT,
_name: Union[str, DefaultType] = DEFAULT,
query: Union[str, float, bool, DefaultType] = DEFAULT,
analyzer: Union[str, DefaultType] = DEFAULT,
auto_generate_synonyms_phrase_query: Union[bool, DefaultType] = DEFAULT,
cutoff_frequency: Union[float, DefaultType] = DEFAULT,
fuzziness: Union[str, int, DefaultType] = DEFAULT,
fuzzy_rewrite: Union[str, DefaultType] = DEFAULT,
fuzzy_transpositions: Union[bool, DefaultType] = DEFAULT,
lenient: Union[bool, DefaultType] = DEFAULT,
max_expansions: Union[int, DefaultType] = DEFAULT,
minimum_should_match: Union[int, str, DefaultType] = DEFAULT,
operator: Union[Literal["and", "or"], DefaultType] = DEFAULT,
prefix_length: Union[int, DefaultType] = DEFAULT,
zero_terms_query: Union[Literal["all", "none"], DefaultType] = DEFAULT,
boost: Union[float, DefaultType] = DEFAULT,
_name: Union[str, DefaultType] = DEFAULT,
Copy link
Member

Choose a reason for hiding this comment

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

Go home GitHub, you're drunk?

Comment on lines 1179 to 1185
"""
A single bucket aggregation that narrows the set of documents to those
that match a query.

:arg filter: A single bucket aggregation that narrows the set of
documents to those that match a query.
"""
Copy link
Member

Choose a reason for hiding this comment

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

I love how we're keeping essentially the same code but adding documentation!

return FieldBucketData(self, search, data)


class RandomSampler(Bucket[_R]):
Copy link
Member

Choose a reason for hiding this comment

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

RandomSampler is in the specification now. I've just backported to 8.15 in case it helps here.

@miguelgrinberg miguelgrinberg added the backport 8.x Backport to 8.x label Oct 4, 2024
@miguelgrinberg miguelgrinberg merged commit ec8da55 into elastic:main Oct 4, 2024
17 checks passed
@miguelgrinberg miguelgrinberg deleted the generate-aggregations branch October 4, 2024 15:25
Copy link

github-actions bot commented Oct 4, 2024

The backport to 8.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.x 8.x
# Navigate to the new working tree
cd .worktrees/backport-8.x
# Create a new branch
git switch --create backport-1918-to-8.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ec8da5577f5c127a0c8fba9b0bfcfabb3e5eb171
# Push it to GitHub
git push --set-upstream origin backport-1918-to-8.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.x

Then, create a pull request where the base branch is 8.x and the compare/head branch is backport-1918-to-8.x.

@miguelgrinberg miguelgrinberg added backport 8.x Backport to 8.x and removed backport 8.x Backport to 8.x labels Oct 7, 2024
github-actions bot pushed a commit that referenced this pull request Oct 7, 2024
* auto-generate aggregation classes

* feedback

(cherry picked from commit ec8da55)
miguelgrinberg added a commit that referenced this pull request Oct 7, 2024
* auto-generate aggregation classes

* feedback

(cherry picked from commit ec8da55)

Co-authored-by: Miguel Grinberg <miguel.grinberg@gmail.com>
miguelgrinberg added a commit to miguelgrinberg/elasticsearch-dsl-py that referenced this pull request Dec 9, 2024
* auto-generate aggregation classes

* feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 8.x Backport to 8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants