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

regression(mssql): investigate bringing back any/all and cumulative versions #8073

Closed
1 task done
cpcloud opened this issue Jan 23, 2024 · 0 comments · Fixed by #8409
Closed
1 task done

regression(mssql): investigate bringing back any/all and cumulative versions #8073

cpcloud opened this issue Jan 23, 2024 · 0 comments · Fixed by #8409
Assignees
Labels
mssql The Microsoft SQL Server backend regression Issues related to things that used to work but don't anymore tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split`

Comments

@cpcloud
Copy link
Member

cpcloud commented Jan 23, 2024

What happened?

These are now failing on the-epic-split

What version of ibis are you using?

the-epic-split

What backend(s) are you using, if any?

mssql

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@cpcloud cpcloud added mssql The Microsoft SQL Server backend regression Issues related to things that used to work but don't anymore tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split` labels Jan 23, 2024
@gforsyth gforsyth self-assigned this Feb 20, 2024
gforsyth added a commit that referenced this issue Feb 21, 2024
## Description of changes

I took a look through what sqlalchemy was generating for these failing
test cases and then reimplemented it using sqlglot.

Any and All:
* Instead of `logical or` we do a `Max(IIF(condition, 1, 0))`
* Instead of `logical and` we do a `Min(IIF(condition, 1, 0))`

Cumulative versions:

The cumulative versions work, but required an additional check in the
`visit_Not` node. MSSQL doesn't support doing `IFF(MAX(IFF(...` -- the
argument to IFF has to be a boolean and the Window function isn't
considered a boolean for some reason, so the outer conditional needs to
be a case statement.- fix(mssql): restore any, all and cumulative
versions
- fix(mssql): support aggregations with any/all


## Issues closed

* Resolves #8073
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mssql The Microsoft SQL Server backend regression Issues related to things that used to work but don't anymore tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split`
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants