SNOW-2680714 : Fix time_series_agg() correctness and make it GA#4049
SNOW-2680714 : Fix time_series_agg() correctness and make it GA#4049sfc-gh-rsureshbabu wants to merge 7 commits intomainfrom
Conversation
|
|
||
| return df | ||
|
|
||
| @experimental(version="1.12.0") |
There was a problem hiding this comment.
do this deserve a line in the CHANGELOG?
There was a problem hiding this comment.
Updated.
| aggs: Dict[str, List[str]], | ||
| windows: List[str], | ||
| group_by: List[str], | ||
| sliding_interval: str = "", |
There was a problem hiding this comment.
I'm afraid we can not remove a positional argument easily which is a BCR.
even though this is marked as an experimental feature, but since it's introduced in 1.12.0, people might already use the function in production.
we will have to keep this field until we have a snowpark v2.0 release which can introduce BCRs
There was a problem hiding this comment.
Like we discussed offline. This PR doesn't remove the argument. it only fixes the correctness issue and makes it GA for now. we will remove the argument as part of BCR release
| repeated string formatted_col_names = 3; | ||
| repeated string group_by = 4; | ||
| string sliding_interval = 5; | ||
| reserved 5; // sliding_interval removed in 1.32.0 |
There was a problem hiding this comment.
the ast is defined in the mono repo, and the file here is generated from the mono repo definition.
if we want to remove the field, the updates need to be happen in the mono repo first.
however, as I pointed in the earlier that removing positional argument is a BCR
|
|
||
| @experimental(version="1.12.0") | ||
| @publicapi | ||
| def time_series_agg( |
There was a problem hiding this comment.
I'm curious how changing time_series_agg fixes the issue SNOW-2680714. Are we calling this function with the sliding_window_agg or is the customer passing this in?
There was a problem hiding this comment.
Made recent updates, Please take a look. We will exclude the rows with same timestamp with second granularity.
sfc-gh-rsureshbabu
force-pushed
the
rsureshbabu-SNOW-2680714-timeaggfuncfix
branch
from
6dcad6f to
3d4218c
Compare
…t row Breaking change in v1.45.0: - Changed window frame to exclude current row to prevent data leakage in ML use cases - Past windows: now use -interval to 1 PRECEDING (was: -interval to CURRENT ROW) - Future windows: now use 1 FOLLOWING to interval (was: CURRENT ROW to interval) - Updated all integration tests with new expected values - Updated proto file with breaking change comment - Updated CHANGELOG with breaking change section
sfc-gh-rsureshbabu
changed the title
sfc-gh-rsureshbabu
requested review from
sfc-gh-jrose,
sfc-gh-mayliu and
sfc-gh-yixie
4 participants
Fixes SNOW-2680714
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
This PR removes the deprecated
sliding_intervalparameter from thetime_series_aggfunction in the DataFrameAnalyticsFunctions class. The parameter was previously marked as deprecated in version 1.31.0 and has now been completely removed from the function signature and AST proto definition. The@experimentaldecorator has also been removed from the function, making it a fully supported API. Tests have been updated to reflect these changes.