ENH: Namespace time_col config, fix type check for trim_with_timecontext for pandas window execution. #2680
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
icexelloss
reviewed
Mar 11, 2021
icexelloss
reviewed
Mar 11, 2021
|
|
||
| # Filter the data, here we preserve the time index so that when user is | ||
| # computing a single column, the computation and the relevant time | ||
| # indexes are returned. | ||
| time_col = get_time_col() | ||
| if time_col not in df or not is_datetime64_dtype(df[time_col]): | ||
| if time_col not in df or not is_datetime64_any_dtype(df[time_col]): |
There was a problem hiding this comment.
Do we have test for this
There was a problem hiding this comment.
Removed this, not hitting by any user level APIs
icexelloss
reviewed
Mar 11, 2021
| @@ -95,7 +95,7 @@ def test_context_adjustment_window_udf( | |||
| """ This test case aims to test context adjustment of | |||
| udfs in window method. | |||
| """ | |||
| with option_context('time_col', 'timestamp_col'): | |||
| with option_context('timecontext.time_col', 'timestamp_col'): | |||
There was a problem hiding this comment.
should be under a the namespace context_adjustment
There was a problem hiding this comment.
How about timecontext_adjustment? We have been calling this timecontext instead of context all over the place
There was a problem hiding this comment.
Use context_adjustment as suggested
LeeTZ
changed the title
LeeTZ
force-pushed
the
tiezheng/namespace_time_context_col_name
branch
from
4148fb9
to
c172631
Compare
LeeTZ
force-pushed
the
tiezheng/namespace_time_context_col_name
branch
from
71116d5
to
6e36706
Compare
LeeTZ
changed the title
|
thanks @LeeTZ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
As a follow-up of #2646, add namespace
timecontextfor the ibis configtime_col.Also, fix a type check in
trim_with_time_contextfor pandas window execution. The type check to check a DataFrame has atime_colcolumn and its type is a Datetime type, will not hit by any user level APIs, removing it.For the result series of a Groupby Transform. The result series is still using the name of the input series. We should unset it to avoid confusion. e.g. If the input series is 'time' and the result series is float but still named 'time' Then in trimming with context the series will be identified as a time column and result in an error.
Tests
Still using the tests implemented in
ibis/backends/tests/test_timecontext.py, andibis/backends/execution/pandas/test_timecontext.py