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

bug: Table.select(col=<literal>).order_by("col") references the value, not column name, in SQL #10428

Open
1 task done
NickCrews opened this issue Nov 4, 2024 · 6 comments · May be fixed by #10475
Open
1 task done

bug: Table.select(col=<literal>).order_by("col") references the value, not column name, in SQL #10428

NickCrews opened this issue Nov 4, 2024 · 6 comments · May be fixed by #10475
Labels
bug Incorrect behavior inside of ibis duckdb The DuckDB backend

Comments

@NickCrews
Copy link
Contributor

What happened?

import ibis

t = ibis.memtable({"a": [1, 2, 3]})
s = t.select("a", i=9, s=ibis.literal("foo"))
o = s.order_by("a", "i", "s")
ibis.to_sql(o)

creates

SELECT
  "t0"."a",
  9 AS "i",
  'foo' AS "s"
FROM "ibis_pandas_memtable_ihuncvko6jcbjbtnxm2cdvekru" AS "t0"
ORDER BY
  "t0"."a" ASC,
  9 ASC,
  'foo' ASC

I would expect

SELECT
  "t0"."a",
  9 AS "i",
  'foo' AS "s"
FROM "ibis_pandas_memtable_ihuncvko6jcbjbtnxm2cdvekru" AS "t0"
ORDER BY
  "t0"."a" ASC,
  "t0"."i" ASC,
  "t0"."s" ASC

What version of ibis are you using?

main

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

No response

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@NickCrews NickCrews added the bug Incorrect behavior inside of ibis label Nov 4, 2024
@gforsyth gforsyth added the duckdb The DuckDB backend label Nov 9, 2024
@gforsyth
Copy link
Member

gforsyth commented Nov 9, 2024

that's a weird one. thanks for reporting @NickCrews !

@cpcloud
Copy link
Member

cpcloud commented Nov 11, 2024

@NickCrews Do you have an example where this deferencing produces an incorrect computation?

As you've currently reported it I don't see a correctness issue here.

@cpcloud cpcloud added question Questions about the library and removed bug Incorrect behavior inside of ibis labels Nov 11, 2024
@NickCrews
Copy link
Contributor Author

If you run that SQL in duckdb it errors

@cpcloud
Copy link
Member

cpcloud commented Nov 11, 2024 •
edited
Loading

Ah, right the 9 doesn't mean the same thing as the literal number 9 when used in an ORDER BY.

@cpcloud cpcloud linked a pull request Nov 12, 2024 that will close this issue
Open
@kszucs
Copy link
Member

kszucs commented Nov 14, 2024

The selections should be replaced in the sort keys to properly reference the projected values in the order by clause. While #10475 solves this by implementing an optimization, a more robust solution would be to reference the projected values whenever we can.

@cpcloud
Copy link
Member

cpcloud commented Nov 14, 2024

The selections should be replaced in the sort keys to properly reference the projected values in the order by clause. While #10475 solves this by implementing an optimization, a more robust solution would be to reference the projected values whenever we can.

The problem with that solution is that not all of our backends support referencing projected columns in the ORDER BY, some require repeating the expression.

@NickCrews NickCrews added bug Incorrect behavior inside of ibis and removed question Questions about the library labels Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Incorrect behavior inside of ibis duckdb The DuckDB backend
Projects
Ibis planning and roadmap
Status: backlog
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(sql): remove constants in order_by calls during select merging cpcloud/ibis
4 participants
@cpcloud @kszucs @gforsyth @NickCrews