-
Notifications
You must be signed in to change notification settings - Fork 608
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
feat(snowflake): implement Table.sample #9071
Conversation
A few thoughts/questions I have on this: Should we wait to move this logic to the SQLGlotCompiler? Should we break apart the first sample test? It made it past the first assertion but failed on CC: @cromano8 as we were discussing this behavior. |
Definitely going to check this out tomorrow/wednesday |
I am marking this as a draft for the time being; I haven't been able to dig back in yet. I'm currently working on setting things up to properly test locally against this backend rather than relying on the cloud-ci-runs. This should make future PRs a little smoother for me as well. |
@IndexSeek I can help here, there's only two kinds of failures and I don't think there's anything else to do here:
|
036b683
to
6bd9e12
Compare
6bd9e12
to
c795fc5
Compare
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.
LGTM!
Snowflake sample tests are passing:
|
That's awesome; thank you so much for helping me get this across the finish line!
Ahh, that makes sense! I was digging around to see if I had forgotten to remove it/add it from/to an iterable somewhere. |
This PR aims to add support for SAMPLE/TABLESAMPLE for Snowflake to build on the work done in #7377.
As the SAMPLE/TABLESAMPLE clause seems to be written the same way between Snowflake and DuckDB in SQL, I was able reuse code from:
ibis/ibis/backends/duckdb/compiler.py
Lines 118 to 128 in b48f451
Snowflake Docs: SAMPLE/TABLESAMPLE
Code Snippet I used to validate it was working:
Results: