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

unwrap panic on several queries with let CTEs #2926

Closed
2 tasks done
blainehansen opened this issue Jun 26, 2023 · 11 comments
Closed
2 tasks done

unwrap panic on several queries with let CTEs #2926

blainehansen opened this issue Jun 26, 2023 · 11 comments
Labels
bug Invalid compiler output or panic

Comments

@blainehansen
Copy link

blainehansen commented Jun 26, 2023

What happened?

The below prql causes this unwrap panic:

core/abstract_engine__test.py thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', prql-compiler/src/semantic/lowering.rs:923:40
stack backtrace:
   0: rust_begin_unwind
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:579:5
   1: core::panicking::panic_fmt
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/panicking.rs:64:14
   2: core::panicking::panic
             at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/panicking.rs:114:5
   3: prql_compiler::semantic::resolve
   4: prql_compiler::pl_to_rq
... python/py03 backtrace ...

It also fails in the playground, giving a Missing query error at the end of input

PRQL input

let customer = (
	from customer
)

from txn

SQL output

Panics.

Expected SQL output

Shouldn't panic.

MVCE confirmation

  • Minimal example
  • New issue

Anything else?

Version: prql_python = "^0.8.1"

I'll be happy to pitch in to solve this. We're using prql as an intermediate step from a custom dsl, and obviously the actual queries we want to compile aren't this trivial, but this is a minimal example. Here's an example query that's a little more realistic:

let customer = (
	from customer
	filter s"(age between 20 and 40)"
)

from txn
filter s"(date_day between '2020-01-01' and '2020-01-05')"
filter s"(site_id = 1)"
join side:inner customer [txn.customer_id == customer.id]
derive [profit = txn.revenue - txn.cost]
group [txn.date_day] (aggregate [cost = sum txn.cost, profit = sum profit, revenue = sum txn.revenue])
@blainehansen blainehansen added the bug Invalid compiler output or panic label Jun 26, 2023
@max-sixty
Copy link
Member

max-sixty commented Jun 27, 2023

Thanks @blainehansen !

I see it working OK on main — would you be OK to confirm on your end? (Note that the list / tuple syntax has changed a bit in 0.9.0)

If you're exclusively on python and don't have a rust compiler it's not trivial to test. But we will be releasing 0.9.0 fairly soon... #2689

@max-sixty
Copy link
Member

max-sixty commented Jun 27, 2023

Also not what you asked, but check out in!

echo "from customer
   filter (age | in 20..40)
   filter (dt | in @2000-01-01..@2020-01-01)" | cargo run -p prqlc -- compile -

SELECT
  *
FROM
  customer
WHERE
  age BETWEEN 20 AND 40
  AND dt BETWEEN DATE '2000-01-01' AND DATE '2020-01-01'

@blainehansen
Copy link
Author

Hey! Sorry didn't have time to think about this for a few days.

I have a rust compiler on my machine, but our internal project is a pure python codebase built using poetry/docker :/
I'm going to try compiling the prql-python crate and using docker volume to make prql_python.so available in that environment.

That's probably not going to cut it for our production build though hahahahah. When do you think 0.9.0 will be released?

@blainehansen
Copy link
Author

Yeah I can't even get it to work because of differences in python version between my system and the docker image :( (ld has stymied me for now, I don't have time to keep looking into this).

I believe you that it works on main ha 🤷‍♂️ I just need a way to get that change into our build process. An alpha version published to pypi would certainly solve my problem :)

P.S. As a hail mary, I also tried both of these dependency statements in our pyproject.toml, and neither worked :(

prql_python = { git = "git@github.com:PRQL/prql.git", subdirectory = "bindings/prql-python" }
prql_python = { git = "git@github.com:PRQL/prql.git" }

@max-sixty
Copy link
Member

Does a plain pip install work for you? This works for me:

pip install "git+https://github.com/prql/prql.git#egg=prql-python&subdirectory=bindings/prql-python"

Or possibly it sounds like it needs to be installed it in your docker image, but that image doesn't have a linker, which is required since this is compiling from source?

@max-sixty
Copy link
Member

I could finish off #1618, and then we could publish wheels...

@blainehansen
Copy link
Author

You know what, I'm realizing this isn't as urgent as I'm making it sound hahaha. If you guys have figured out this problem in a version that will be released in the next few weeks, that's just fine. (I'm only working on a speculative project, it will become my main focus but not for a few weeks.)

Don't sweat this for now, just release 0.9.0 when you can :)

P.S., I tried the manual pip install while inside the docker container (had to install rustup in it first ha), and it still didn't work, but most likely just because the pip install didn't put things in the same place as everything else ha. Again, don't sweat it. I've verified the queries are fine when I run the compiler through cargo run in my normal machine environment :)

@eitsupi
Copy link
Member

eitsupi commented Jun 30, 2023

Note that nightly builds include prql-python wheels (e.g. https://github.com/PRQL/prql/actions/runs/5411366409)

I believe this could be published to PyPI. (the problem is that there is no mechanism to determine the version number)

@max-sixty
Copy link
Member

max-sixty commented Jul 26, 2023

I believe this could be published to PyPI. (the problem is that there is no mechanism to determine the version number)

Python supports versioning by the git hash, or the number of commits since the previous tag — so we could use https://github.com/pypa/setuptools_scm to do this...

@eitsupi
Copy link
Member

eitsupi commented Jul 26, 2023

Python supports versioning by the git hash, or the number of commits since the previous tag — so we could use pypa/setuptools_scm to do this...

Can it be used in combination with maturin? I believe prql-python currently uses the Cargo.toml version as is.

@max-sixty
Copy link
Member

Can it be used in combination with maturin? I believe prql-python currently uses the Cargo.toml version as is.

Sorry, no it can't, at least with no luck with 10 mins of testing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Invalid compiler output or panic
Projects
None yet
Development

No branches or pull requests

3 participants