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

sql: support attribute notation for single-param functions (x.f equivalent to f(x)) #47477

Open
rafiss opened this issue Apr 14, 2020 · 5 comments
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-activerecord C-investigation Further steps needed to qualify. C-label will change. T-sql-queries SQL Queries Team

Comments

@rafiss
Copy link
Collaborator

rafiss commented Apr 14, 2020

This came up during development of the ActiveRecord adapter for CockroachDB. Part of the tool relies on this Postgres behavior. (It's not critical to functionality, but just means some queries may not work.)

rafiss@127:postgres> create table tab (pk int primary key, s varchar, i int);
CREATE TABLE
rafiss@127:postgres> insert into tab values (1, 'a', 1), (2, 'a', 3), (3, 'b', 5);
INSERT 0 3
rafiss@127:postgres> select s, max(i) from tab group by s order by tab.count desc;
+-----+-------+
| s   | max   |
|-----+-------|
| a   | 3     |
| b   | 5     |
+-----+-------+

In CockroachDB, the above results in ERROR: column "tab.count" does not exist

Here is the ActiveRecord test

  def test_group_by_with_order_by_virtual_count_attribute
    expected = { "SpecialPost" => 1, "StiPost" => 2 }
    actual = Post.group(:type).order(:count).limit(2).maximum(:comments_count)
    assert_equal expected, actual
  end if current_adapter?(:PostgreSQLAdapter)

Jira issue: CRDB-4406

@rafiss rafiss added the A-sql-pgcompat Semantic compatibility with PostgreSQL label Apr 14, 2020
@rytaft
Copy link
Collaborator

rytaft commented Apr 14, 2020

cc @RaduBerinde, @awoods187

@RaduBerinde
Copy link
Member

It looks like this is some special syntax that allows interpreting x.f as f(x), combined with the fact that postgres allows count(table) (as opposed to count(table.*)).

AFAICT it is always equivalent to use count(*) or count(t.*). I this case I believe the code could just do .order("COUNT(*)") instead of .order(:count).

I would push back against implementing this unless the workaround I mentioned doesn't work or we have a strong reason to support existing code that does this.

@awoods187 awoods187 added the C-investigation Further steps needed to qualify. C-label will change. label Apr 14, 2020
@rafiss
Copy link
Collaborator Author

rafiss commented Apr 14, 2020

This section of the Postgres docs covers the behavior that Radu describes: https://www.postgresql.org/docs/12/rowtypes.html#ROWTYPES-USAGE

I'll rename the issue to make it clear it's more of a syntax thing, and not actually a new column.

And some other relevant reading:
https://dba.stackexchange.com/questions/129410/order-by-count-and-bitmap-heap-scan
https://stackoverflow.com/questions/11165450/store-common-query-as-column/11166268#11166268

@rafiss rafiss changed the title sql: aggregates should make an implicit count output column sql: support attribute notation for single-param functions (x.f equivalent to f(x)) Apr 14, 2020
@jlinder jlinder added the T-sql-queries SQL Queries Team label Jun 16, 2021
@mgartner
Copy link
Collaborator

@vy-ton I'm moving to the backlog, but if ActiveRecord is a high priority for 22.2, then let me know so I can reprioritize this.

@mgartner mgartner moved this to Backlog (DO NOT ADD NEW ISSUES) in SQL Queries Jul 24, 2023
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

@mgartner mgartner moved this from Backlog (DO NOT ADD NEW ISSUES) to Cold Storage in SQL Queries Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-activerecord C-investigation Further steps needed to qualify. C-label will change. T-sql-queries SQL Queries Team
Projects
Status: Cold Storage
Development

No branches or pull requests

6 participants