8000 feat: do not add a group by clause to O2O-edges for order by neighbor… by michaelcaulley · Pull Request #3943 · ent/ent · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: do not add a group by clause to O2O-edges for order by neighbor… #3943

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: do not add a group by clause to O2O-edges for order by neighbor… #3943

wants to merge 1 commit into from
+6 −4

Conversation

michaelcaulley
Copy link
Contributor

… terms

this prevents issues with databases that require all selected terms be included in the group by clause. the group by clause is not required if the relationship is O2O, because only one row will be returned if we join on fks.

@crossworth crossworth mentioned this pull request Feb 10, 2024
Closed
@michaelcaulley
Copy link
Contributor Author

@giautm can you help review this change?

a8m
a8m reviewed Feb 11, 2024
View reviewed changes
dialect/sql/sqlgraph/graph.go
GroupBy(toT.C(s.Edge.Columns[0]))
From(toT)
if s.Edge.Rel != O2O {
join = join.GroupBy(toT.C(s.Edge.Columns[0]))
Copy link
Member

Choose a reason for hiding this comment

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

That’s true if the edge is unique, not only o2o.

Also, please add/update tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@a8m Do you have ideas for tests? It looks like there is already a test that covers ordering by a unique edge field, but all the tests use sqlite, and therefore this specific test passed both before and after this change. This change is only to generate valid sql in other databases too, such as PostgreSQL.

@michaelcaulley michaelcaulley force-pushed the order-by-neighbor-terms-o2o-edge-update branch from b03b063 to 9d599d5 Compare September 6, 2024 14:10
@michaelcaulley
feat: do not add a group by clause to O2O-edges for order by neighbor…
ee6caa7 
… terms

this prevents issues with databases that require all selected terms be included
in the group by clause. the group by clause is not required if the relationship
is O2O, because only one row will be returned if we join on fks.
@michaelcaulley michaelcaulley force-pushed the order-by-neighbor-terms-o2o-edge-update branch from 9d599d5 to ee6caa7 Compare April 9, 2025 01:10
@TargetLiu
Copy link
Contributor
TargetLiu commented May 6, 2025 •
edited
Loading

This modification is extremely helpful. In PostgreSQL, attempting to use ORDER BY on an edge results in an error: "pq: column 'xxx' must appear in the GROUP BY clause or be used in an aggregate function". I believe this is a bug that requires urgent resolution. Thank you very much. @a8m

@akushsky
Copy link
akushsky commented May 8, 2025 •
edited
Loading

I'm waiting for that fix too, have the same issue when trying to add ORDER BY edge field to my schema.

Edge to:

edge.To("stats", StoredPackageVersionStats.Type).
			Annotations(entgql.OrderField("STATS_DOWNLOAD_COUNT")).
			Unique(),

Edge from:

func (StoredPackageVersionStats) Fields() []ent.Field {
	return []ent.Field{
		field.Int("id").Unique(),
		field.Int("version_id"),
		field.Int("download_count").Annotations(entgql.OrderField("DOWNLOAD_COUNT")),
	}
}

func (StoredPackageVersionStats) Edges() []ent.Edge {
	return []ent.Edge{
		edge.
			From("version", StoredPackageVersion.Type).
			Field("version_id").
			Ref("stats").Required().Unique(),
	}
}

Bad SQL in result:

SELECT "md_versions"."id", "md_versions"."name", "t1"."stats_download_count" FROM "md_versions" LEFT JOIN (SELECT "md_ver_stats"."version_id", "md_ver_stats"."download_count" AS "stats_download_count" FROM "md_ver_stats" GROUP BY "md_ver_stats"."version_id") AS "t1" ON "md_versions"."id" = "t1"."version_id" ORDER BY "t1"."stats_download_count" NULLS FIRST, "md_versions"."id" LIMIT 51

@a8m

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a8m a8m a8m left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michaelcaulley @TargetLiu @akushsky @a8m
0