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

(PDB-5278) Fix group by dotted fact path with forward slash #3573

Open
wants to merge 1 commit into
base: 6.x
Choose a base branch
from

Conversation

austb
Copy link
Contributor

@austb austb commented Sep 17, 2021

When grouping by a keyword, honeysql will convert the keyword to SQL by
calling name on it. But that will not return the entire fact name when
there's a forward slash in it because Clojure interprets everything
before the forward slash as the "namespace" and everything after as the
"name".

=> (name :facts.foo)
"facts.foo"
=> (name :facts.f/oo)
"oo"
=> (namespace :facts.f/oo)
"facts.f"

This commit changes the query engine to use sql raw instead of the
keyword to avoid splitting on forward slashes.

@austb austb requested a review from a team as a code owner September 17, 2021 18:45
@sebastian-miclea
Copy link
Contributor

I had a look over this PR and tested on PDB-5262 and that case is also resolved.

@austb austb force-pushed the pdb-5278/6.x/group-by-dotted-with-slash branch from 976f6bb to c5ab459 Compare November 19, 2021 16:15
When grouping by a keyword, honeysql will convert the keyword to SQL by
calling `name` on it. But that will not return the entire fact name when
there's a forward slash in it because Clojure interprets everything
before the forward slash as the "namespace" and everything after as the
"name".

```
=> (name :facts.foo)
"facts.foo"
=> (name :facts.f/oo)
"oo"
=> (namespace :facts.f/oo)
"facts.f"
```

This commit changes the query engine to use sql raw instead of the
keyword to avoid splitting on forward slashes.
@austb austb force-pushed the pdb-5278/6.x/group-by-dotted-with-slash branch from c5ab459 to 8103004 Compare December 2, 2021 18:27
@CLAassistant
Copy link

CLAassistant commented Apr 19, 2023

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants