-
Notifications
You must be signed in to change notification settings - Fork 122
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-1936603: fix limit 0 bug with show #3090
base: main
Are you sure you want to change the base?
Conversation
28c2209
to
2bc8238
Compare
22ebeca
to
47ac899
Compare
hey @sfc-gh-fhe , the local testing tests are failing |
CHANGELOG.md
Outdated
@@ -15,6 +15,7 @@ | |||
|
|||
- Fixed a bug where creating a Dataframe with large number of values raised `Unsupported feature 'SCOPED_TEMPORARY'.` error if thread-safe session was disabled. | |||
- Fixed a bug where `df.describe` raised internal SQL execution error when the dataframe is created from reading a stage file and CTE optimization is enabled. | |||
- Fixed a bug where `df.show` would still show rows after a `df.limit(0)` call was applied to it. |
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.
This isn't just for df.show()
. It's for df.limit()
. I suggest updating this description.
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.
This worked fine for df.collect()
and df.count()
. I believe it was just a df.show()
issue
8490f43
to
1d8725d
Compare
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1936603
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
When checking to use self.limit_ or n in the select statement limit function, we first check if self.limit_ is true. The issue with that is if self.limit_ is 0, it always evaluates to false and uses n instead.