-
Notifications
You must be signed in to change notification settings - Fork 106
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
SNOW-1458133 Implement Index.value_counts
#1902
SNOW-1458133 Implement Index.value_counts
#1902
Conversation
Index.value_counts
Index.value_counts
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
Outdated
Show resolved
Hide resolved
…new method in snowflake query compiler.
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
Outdated
Show resolved
Hide resolved
…counts # Conflicts: # CHANGELOG.md
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
Show resolved
Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
Outdated
Show resolved
Hide resolved
…counts # Conflicts: # tests/integ/modin/index/test_index_methods.py
…t implemented bins
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
Outdated
Show resolved
Hide resolved
src/snowflake/snowpark/modin/plugin/compiler/snowflake_query_compiler.py
Outdated
Show resolved
Hide resolved
… change error from not implemented to assert
…counts # Conflicts: # CHANGELOG.md
@@ -1433,21 +1433,20 @@ def value_counts( | |||
normalize: bool = False, | |||
sort: bool = True, | |||
ascending: bool = False, | |||
bins: Any = None, | |||
bins: int | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bins: int | None = None, | |
bins: Optional[int] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter changes it to int | None
whenever I use Optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one minor q
@@ -5,6 +5,8 @@ The following table is structured as follows: The first column contains the meth | |||
The second column is a flag for whether or not there is an implementation in Snowpark for | |||
the method in the left column. | |||
|
|||
Currently, there is no MultiIndex support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we don't support any operations on MultiIndex objects? We definitely do have APIs that return them, at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, we don't support any operations on MultiIndex since we don't have an object representing them. The lazy Index object we currently have is only supposed to be a single Index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is still worth clarifying, since it might imply to users that DF/Series operations cannot return MultiIndex objects, even though they're just returning eager pandas.MultiIndex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose I'll say that there is no lazy MultiIndex support. This Index object is supposed to only be a single Index but existing DataFrame and Series APIs may support native pandas MultiIndex operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be in now if you want to look at the text
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1458133
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Implemented
Index.value_counts
reusing theSeries.value_counts
implementation.Examples: