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.

Sign up for GitHub

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

Jump to bottom

fix: Raise an error on unavailable excludes #2342

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

fix: Raise an error on unavailable excludes #2342

wants to merge 21 commits into from

Conversation

max-sixty
Copy link
Member

@max-sixty max-sixty commented Mar 29, 2023 •
edited
Loading

Closes #2292.

But I now realize was here deliberately (the old code was more work than mine!)

That said I would think it's better to raise here — getting this wrong is still wrong...

@max-sixty
fix: Raise an error on unavailable excludes
3281b24 
Closes PRQL#2292. It was here deliberately (the old code was _more_ work). I would think it's better to raise here — getting this wrong is still wrong...
@max-sixty
Copy link
Member Author

Tests are failing because of #2079, so I guess we need to solve that first

aljazerzen
aljazerzen approved these changes Apr 2, 2023
View reviewed changes
Copy link
Member

@aljazerzen aljazerzen left a comment

Choose a reason for hiding this comment

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

I did not want throw an error here, because we don't have an easy escape path. (see the comment that was removed).

I've pushed a test that exposes this error when using group (take). If you are ok with throwing an error even in this case, we can merge.

For better errors, we'd need to include more spans in RQ.

max-sixty and others added 15 commits April 2, 2023 14:16
@max-sixty
refactor: Rename set to type (PRQL#2346)
0ec22c4
@dependabot @max-sixty
chore: bump clap from 4.1.10 to 4.2.0 (PRQL#2345)
cc1f3b9 
* chore: bump clap from 4.1.10 to 4.2.0

Bumps [clap](https://github.com/clap-rs/clap) from 4.1.10 to 4.2.0.
- [Release notes](https://github.com/clap-rs/clap/releases)
- [Changelog](https://github.com/clap-rs/clap/blob/master/CHANGELOG.md)
- [Commits](clap-rs/clap@v4.1.10...clap_complete-v4.2.0)

---
updated-dependencies:
- dependency-name: clap
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Maximilian Roos <m@maxroos.com>
@Jelenkee @eitsupi
@max-sixty @pre-commit-ci
test: more RDBMS integration tests in CI (PRQL#2286)
03c335d 
* -added docker compose
-added test skeleton

* -added dependencies

* -added real test cases

* -added mssql

* refactoring

* -added go-task

* -more testcases

* -added github workflow

* -fixed formatting

* -fixed linting

* -fixed workflow yaml

* -fixed workflow yaml again

* -added more testcases

* Apply suggestions from code review

Co-authored-by: eitsupi <50911393+eitsupi@users.noreply.github.com>

* -refactored test workflow

* Fix order of args to `assert_display_snapshot`

Sorry for the incorrect suggestion prior!

* Specify platform for mysql image

* -added csv import

* -fixed or skipped all prql files

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* -fixed sqlite

* added snaps§

* Update prql-compiler/prqlc/src/cli.rs

* -removed test on macOS

* empty

* -skip integration test on windows

* -fixed taskfile

* -added double quotes

* Update .github/workflows/test-rust.yaml

---------

Co-authored-by: eitsupi <50911393+eitsupi@users.noreply.github.com>
Co-authored-by: Maximilian Roos <m@maxroos.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
@eitsupi @max-sixty
chore(test): bind mount read only in compose file for testing (PRQL#2352
4e0b576 
)

* test: bind mount read only in compose file for testing

* chore: should not chown /tmp/chinook
@eitsupi @max-sixty
build: tweak megalinter config for sql (PRQL#2349)
afb6e5e 
* build: disable tsqllint in megalinter

* build: add sqlfluff config file

* format: autoformatting by sqlfluff

* build: ignore `references.keywords` rule of sqlfluff
@max-sixty
refactor: Use tags for the language code blocks (PRQL#2350)
3aa52a8 
* refactor: Use tags for the language code blocks

Idea from PRQL#2348

* change to spaces
@Jelenkee @pre-commit-ci @max-sixty
docs: Update CHANGELOG.md (PRQL#2356)
16e7c72 
* Update CHANGELOG.md

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update CHANGELOG.md

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
@max-sixty
.
113d08c
@max-sixty
internal: Remove unneeded expression (PRQL#2360)
c365137 
We can't hit this since the `*` case is covered above IIUC
@eitsupi @max-sixty
chore(test): set MySQL's secure-file-priv="" option in the command …
5ae5ac4 
…instead of conf file (PRQL#2359)

* chore(test): remove unused mysql conf file

* fix: pass `--secure-file-priv=""` via compose file
@max-sixty
chore: Add changelog for 0.7.0 (PRQL#2363)
ee066d4
@max-sixty
chore: Bump versions to 0.7.0 (PRQL#2364)
41f6321
@max-sixty
chore: Add changelog template for 0.7.1 (PRQL#2365)
66c65bf
@eitsupi @max-sixty @pre-commit-ci
docs(book): include PRQL changelog in book (PRQL#2348)
414fd71 
Close PRQL#2344

Since `prql` code blocks in md files included in the Book will evaluated
and replace with html, this PR introduce a new Option `no-eval` that
modifies the Book preprocessor to not evaluate Changelog's `prql` code
blocks.
Also add a pre-commit hook to avoid using bare `prql` code blocks on
`CHANGELOG.md`.

---------

Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@max-sixty
Merge branch 'main' into 2292
b72b229
@max-sixty
Copy link
Member Author

I did not want throw an error here, because we don't have an easy escape path. (see the comment that was removed).

There are two cases I can see:

  • The group 1 — i.e. the test you added — I hadn't thought we had to throw an error there (assuming we wrote perfect code) — is that not correct? That query could compile to SELECT DISTINCT title FROM tracks, no?
  • The case where someone has specified a column to exclude. I thought that was what the comment was referring to. My vote would be to raise an error there — that the downside of the "no escape path" was lower than the downside of "I asked for the exclude and it just ignored me"...

@aljazerzen
Copy link
Member

The group 1 — i.e. the test you added — I hadn't thought we had to throw an error there (assuming we wrote perfect code) — is that not correct? That query could compile to SELECT DISTINCT title FROM tracks, no?

When there is take 1 it should yes, but group x (take 2) should not.

There may be other cases where a column is added and should later be removed without removing the unknown columns, but I cannot think of any right now.

@max-sixty
Copy link
Member Author

When there is take 1 it should yes, but group x (take 2) should not.

Good point. Ideally we'd be able to handle those cases differently, I recognize that will require more work though.

I'll leave this open for a bit and see whether I / someone else gets to it. Otherwise we can close and attempt a broader effort.

max-sixty added a commit to max-sixty/prql that referenced this pull request Apr 14, 2023
@max-sixty
internal: Add test module
25a6215 
This has been sitting in PRQL#2342, which likely won't get merged, so adding it here.
@max-sixty max-sixty mentioned this pull request Apr 14, 2023
Merged
max-sixty added a commit that referenced this pull request Apr 14, 2023
@max-sixty
internal: Add test module (#2432)
03e62ef 
This has been sitting in #2342, which likely won't get merged, so adding
it here.
@max-sixty max-sixty mentioned this pull request Oct 7, 2023
Closed
2 tasks
@aljazerzen aljazerzen added the language-design Changes to PRQL-the-language label Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aljazerzen aljazerzen aljazerzen approved these changes

Assignees
No one assigned
Labels
language-design Changes to PRQL-the-language
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

select ! should raise an error if it can't exclude
4 participants
@max-sixty @aljazerzen @Jelenkee @eitsupi