Skip to content

Conversation

@NickCrews
Copy link
Contributor

@NickCrews NickCrews commented Nov 20, 2025

Fixes #6376

  • on postgres, using either IGNORE NULLS or RESPECT NULLS errors
  • on mySQL, you are allowed to use RESPECT NULLS, only using IGNORE NULLS results in an error

There are probably other dialects that also need this change, but these are the ones I've discovered so far.

@geooo109
Copy link
Collaborator

@NickCrews thanks for the PR, it seems that this is the same handling as the SQLite.
Can you also add tests for this? (after fixing the typing)

@NickCrews NickCrews force-pushed the ignore-nulls-unsupported branch from c4a6437 to b3fb79d Compare November 20, 2025 22:11
@NickCrews
Copy link
Contributor Author

NickCrews commented Nov 20, 2025

There are no tests for the sqlite version that I can use as a template. Should I add tests to each individual test_{dialect}.py file? I think it would be better if there was a central test_ignore_nulls() function that was parameterized by the dialect

I also can't reproduce the ruff-format linting error from CI. When I try to repro that using the steps in the GHA workflow, the ruff-format check passes. I'm still getting ruff==0.7.2. I am on macos, not linux, so maybe there is some other environment mismatch? I wish you would use uv and a lockfile in this project, it would be way faster and easier and familiar for me and probably other python devs.

@geooo109
Copy link
Collaborator

Something similar with the unsupported here: d86a114#diff-3d52365bb42988855f191716c88abe51e577bd1122e2a977a5d967281f1b17f9

@georgesittas
Copy link
Collaborator

I also can't reproduce the ruff-format linting error from CI. When I try to repro that using the steps in the GHA workflow, the ruff-format check passes. I'm still getting ruff==0.7.2. I am on macos, not linux, so maybe there is some other environment mismatch? I wish you would use uv and a lockfile in this project, it would be way faster and easier and familiar for me and probably other python devs.

If you dev. in a fresh venv environment and run make install-dev you should get matching dependency versions to fix this issue.

@geooo109
Copy link
Collaborator

@NickCrews let me get this to the finish line

@geooo109 geooo109 closed this Nov 21, 2025
@geooo109 geooo109 reopened this Nov 21, 2025
@geooo109 geooo109 merged commit 90a3fa9 into tobymao:main Nov 21, 2025
4 of 16 checks passed
@NickCrews
Copy link
Contributor Author

Something similar with the unsupported here: d86a114#diff-3d52365bb42988855f191716c88abe51e577bd1122e2a977a5d967281f1b17f9

Oops, wow, sorry, I totally forgot about #5185. If I had, then I would have had a lot better sense of what to do.

@NickCrews let me get this to the finish line

Thank you very much!

If you dev. in a fresh venv environment and run make install-dev you should get matching dependency versions to fix this issue.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should we error when generating first_value(...) IGNORE NULLS for dialects that don't support it?

3 participants