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

SNOW-976702 : Add timeseries agg dataframe function. #1181

Merged
merged 47 commits into from
Feb 2, 2024

Conversation

sfc-gh-rsureshbabu
Copy link
Collaborator

@sfc-gh-rsureshbabu sfc-gh-rsureshbabu commented Dec 13, 2023

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    SNOW-976702

The Pr is adding the time series aggregation function as proposed in https://docs.google.com/document/d/14J5lr_a3fE1xeU-YzKWdeduBdC_035sry11lJUjjkT8/edit

  1. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
  2. Please describe how your code solves the related issue.

    Please write a short description of how your code change solves the related issue.

@sfc-gh-rsureshbabu sfc-gh-rsureshbabu changed the base branch from main to rsureshbabu-SNOW-SNOW-976701-movingagg December 13, 2023 17:38
@sfc-gh-rsureshbabu sfc-gh-rsureshbabu marked this pull request as ready for review December 13, 2023 17:40
@sfc-gh-rsureshbabu sfc-gh-rsureshbabu requested a review from a team as a code owner December 13, 2023 17:40
@sfc-gh-rsureshbabu sfc-gh-rsureshbabu requested review from sfc-gh-mkeller, sfc-gh-yixie, sfc-gh-wezhou and sfc-gh-tbao and removed request for a team December 13, 2023 17:40
Base automatically changed from rsureshbabu-SNOW-SNOW-976701-movingagg to main January 25, 2024 00:15
Comment on lines +456 to +457
sliding_point_col = "sliding_point"

Copy link
Contributor

Choose a reason for hiding this comment

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

would this overwrite sliding_point if original df has this column? I would suggest using randomized name - see random_table_name in utils.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not a temporary column. I am not sure about generating a random name for an output column. This will make it hard to write queries based on this column. We could put a contact suffix/prefix to the name. That can bring down the probability of collision. Say "sp_gen_sliding_point". wdyt ?

@sfc-gh-rsureshbabu sfc-gh-rsureshbabu enabled auto-merge (squash) February 1, 2024 22:31
@sfc-gh-rsureshbabu sfc-gh-rsureshbabu merged commit 20d29e0 into main Feb 2, 2024
57 checks passed
@sfc-gh-rsureshbabu sfc-gh-rsureshbabu deleted the rsureshbabu-SNOW-976702-timeseries_agg branch February 2, 2024 01:11
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants