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

fix(sql): remove constants in order_by calls during select merging #10475

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Nov 12, 2024

Remove literals in ORDER BY when sorting tables. Fixes #10428.

@cpcloud cpcloud added this to the 10.0 milestone Nov 12, 2024
@cpcloud cpcloud added bug Incorrect behavior inside of ibis sql Backends that generate SQL labels Nov 12, 2024
@cpcloud cpcloud changed the title fix(sql): remove constants in order_by fix(sql): remove constants in order_by calls during select merging Nov 12, 2024
@cpcloud cpcloud requested a review from kszucs November 12, 2024 11:43
@github-actions github-actions bot added the tests Issues or PRs related to tests label Nov 12, 2024
@cpcloud
Copy link
Member Author

cpcloud commented Nov 12, 2024

It looks like DuckDB's planner erases constants in ORDER BY:

❯ duckdb <<< "SET order_by_non_integer_literal=true; CREATE TABLE t AS SELECT i FROM RANGE(1, 4) AS _ (i); EXPLAIN SELECT i, 'foo' as x FROM t ORDER BY i DESC, 'foo'"

┌─────────────────────────────┐
│┌───────────────────────────┐│
││       Physical Plan       ││
│└───────────────────────────┘│
└─────────────────────────────┘
┌───────────────────────────┐
│         PROJECTION        │
│    ────────────────────   │
│__internal_decompress_integ│
│     ral_bigint(#0, 1)     │
│__internal_decompress_strin│
│           g(#1)           │
│                           │
│          ~3 Rows          │
└─────────────┬─────────────┘
┌─────────────┴─────────────┐
│          ORDER_BY         │
│    ────────────────────   │
│          t.i DESC         │
└─────────────┬─────────────┘
┌─────────────┴─────────────┐
│         PROJECTION        │
│    ────────────────────   │
│__internal_compress_integra│
│     l_utinyint(#0, 1)     │
│__internal_compress_string_│
│        uinteger(#1)       │
│                           │
│          ~3 Rows          │
└─────────────┬─────────────┘
┌─────────────┴─────────────┐
│         PROJECTION        │
│    ────────────────────   │
│             i             │
│             x             │
│                           │
│          ~3 Rows          │
└─────────────┬─────────────┘
┌─────────────┴─────────────┐
│         SEQ_SCAN          │
│    ────────────────────   │
│             t             │
│                           │
│       Projections: i      │
│                           │
│          ~3 Rows          │
└───────────────────────────┘

@@ -320,7 +320,9 @@ def merge_select_select(_, **kwargs):
selections=selections,
predicates=unique_predicates,
qualified=unique_qualified,
sort_keys=unique_sort_keys,
sort_keys=tuple(
key for key in unique_sort_keys if not isinstance(key.expr, ops.Literal)
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove all expressions which are constant across the relation? I am thinking of all pure scalar nodes.

Copy link
Member Author

@cpcloud cpcloud Nov 14, 2024

Choose a reason for hiding this comment

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

What do you mean? We can't just start deleting anything that's a literal scalar. Expressions like

SELECT 1 as a
FROM t

are 100% valid and useful.

Copy link
Member

Choose a reason for hiding this comment

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

I only mean the order by expressions.

Copy link
Member Author

Choose a reason for hiding this comment

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

No because it's possible to write things like 1 if x else 0, and use them in sort keys.

Copy link
Member

Choose a reason for hiding this comment

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

expr = 1 if x else 0 right, though we can identify that as well, like its .relations attribute is not empty

Copy link
Member

Choose a reason for hiding this comment

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

In any case, it doesn't block this PR.

@gforsyth gforsyth merged commit 6e693b7 into ibis-project:main Dec 4, 2024
77 checks passed
@NickCrews
Copy link
Contributor

Thank you all!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis sql Backends that generate SQL tests Issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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