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-1018248: Add support for asof join in snowpark #1254

Merged

Conversation

sfc-gh-aalam
Copy link
Contributor

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.

    Fixes #SNOW-1018248

  2. 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
  3. Please describe how your code solves the related issue.

    Add ASOF join support in snowpark python.

@sfc-gh-aalam
Copy link
Contributor Author

sfc-gh-aalam commented Feb 17, 2024

https://ci-dev-142.int.snowflakecomputing.com/job/PythonStoredProcBuildPrecommitTest/901/

Add tests have passed on SP side related to ASOF join

@sfc-gh-aalam sfc-gh-aalam requested a review from a team as a code owner February 27, 2024 21:35
Copy link
Contributor

@sfc-gh-jrose sfc-gh-jrose left a comment

Choose a reason for hiding this comment

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

Code looks good.

nit: I'd prefer if the tests were parameterized so that each test case is considered a separate test.

Copy link

@sfc-gh-bolee sfc-gh-bolee left a comment

Choose a reason for hiding this comment

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

Hi, I am not able to understand the code line by line, but just looked at the test cases and it looks good to me. One question though: There are some restrictions on the as of join sql, like match_condition can only have >=, >, <= and < operators, on clause of asof join can have conjuction of equi conditions or so. Will these be relying on the SQL engine? Can we have tests for these cases?

@sfc-gh-aalam
Copy link
Contributor Author

There are some restrictions on the as of join sql, like match_condition can only have >=, >, <= and < operators, on clause of asof join can have conjuction of equi conditions or so. Will these be relying on the SQL engine?

We will rely on sql engine to throw errors for these cases.

Can we have tests for these cases?

Sure. I'll add tests for them

@sfc-gh-aalam sfc-gh-aalam force-pushed the aalam-SNOW-1018248-add-support-for-asof-join-in-snowpark branch from 646ce46 to aacfc37 Compare March 5, 2024 19:20
Copy link

@sfc-gh-bolee sfc-gh-bolee left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you. (Not an expert for the code. only reviewed in high level)

@sfc-gh-aalam sfc-gh-aalam enabled auto-merge (squash) March 6, 2024 23:48
@sfc-gh-aalam sfc-gh-aalam merged commit 7a15479 into main Mar 7, 2024
57 checks passed
@sfc-gh-aalam sfc-gh-aalam deleted the aalam-SNOW-1018248-add-support-for-asof-join-in-snowpark branch March 7, 2024 01:47
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 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.

3 participants