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

feat(hogql): stringOr ^^ #25638

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(hogql): stringOr ^^ #25638

wants to merge 1 commit into from

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Oct 16, 2024

Problem

We want to easily get the first not empty string in Hog/QL:

In JavaScript:

const a = null
const b = ''
const c = 'banana'
const d = 'dingo'

console.log(a || b || c || d) // 'banana' -- this is what we want
console.log(a ?? b ?? c ?? d) // ''

In Hog:

let a := null
let b := ''
let c := 'banana'
let d := 'dingo'

print(a || b || c || d) // 'bananadingo' -- double pipe is string concatenation
print(a ?? b ?? c ?? d) // ''
print(a or b or c or d) // true

In HogQL

select null || '' || 'banana' || 'dingo' // 'bananadingo' -- double pipe is string concatenation
select null ?? '' ?? 'banana' ?? 'dingo' // ''
select null or '' or 'banana' or 'dingo' // errors
select coalesce(null, '', 'banana', 'dingo') // ''

The only real way to get banana is:

let a := null
let b := ''
let c := 'banana'
let d := 'dingo'

print(notEmpty(a) ? a : notEmpty(b) ? b : notEmpty(c) ? c : d) // 'banana'

... but we could do better.

Changes

Introducing stringOr(null, '', 'banana', 'dingo') and its shorthand ^^

image

Why ^^?

Looking at my keyboard, I found these underutilised symbols in our language: ! $ % ^ & _ ~. I excluded symbols that are common bitwise operators or used elsewhere in Hog, and added a bunch of doubles.

The question became: which of these is the most natural/obvious?

print(null ! '' ! 'banana' ! 'dingo')    // feels like "not"
print(null @ '' @ 'banana' @ 'dingo')    // banana at dingo? doesn't read right
print(null $ '' $ 'banana' $ 'dingo')    // somehow this makes me think of joining strings
print(null _ '' _ 'banana' _ 'dingo')    // this too
print(null !! '' !! 'banana' !! 'dingo') // this is "truthiness" in a lot of places
print(null $$ '' $$ 'banana' $$ 'dingo') // feels heavy, and also like concat
print(null %% '' %% 'banana' %% 'dingo') // feels like weird string formatting
print(null ^^ '' ^^ 'banana' ^^ 'dingo') // ^ is XOR, so it is a funky "or" and I can kind of feel it
print(null && '' && 'banana' && 'dingo') // we don't use it, but that's definitely not an "or"
print(null ~~ '' ~~ 'banana' ~~ 'dingo') // it looks like joining with a rope
print(null __ '' __ 'banana' __ 'dingo') // feels like joining/underscoring strings

... and that's how I settled on ^^

How did you test this code?

I didn't yet. This is a WIP in the spirit of "the fastest way to get the right answer is to post the wrong answer on the internet".

To test locally, run pip install ./hogql_parser. If you change the ^^ in HogQLLexer.g4 and want to play around, run pnpm run grammar:build followed by pip install ./hogql_parser.

Alternatives

We claim back ||, however it is a standard SQL concatenation operator. We have advertised it and people are using it in queries, so it'll be a hard fight.

@mariusandra mariusandra requested a review from a team October 16, 2024 19:48
@posthog-bot
Copy link
Contributor

It looks like the code of hogql-parser has changed since last push, but its version stayed the same at 1.0.45. 👀
Make sure to resolve this in hogql_parser/setup.py before merging!

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@@ -1073,6 +1073,7 @@ def compare_types(arg_types: list[ConstantType], sig_arg_types: tuple[ConstantTy
"maxIntersectionsPositionIf": HogQLFunctionMeta("maxIntersectionsPositionIf", 3, 3, aggregate=True),
}
HOGQL_POSTHOG_FUNCTIONS: dict[str, HogQLFunctionMeta] = {
"stringOr": HogQLFunctionMeta("stringOr", 1, None),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just commenting here as good as anywhere.

stringOr sounds really weird to me. Is it truly only for strings? If so then I guess its fine and I assume the naming is coming from logicalOr? Something like firstNonEmptyString() is obviously quite verbose but stringOr didn't mean a whole lot to me at first - could be my lack of common programming terms here.

If it is only for strings then I'm not so sure about adding it as a standard shorthand ^^ (I can't even find that char on my german keyboard 😅 ).

@mariusandra mariusandra removed the stale label Nov 8, 2024
@mariusandra mariusandra removed the stale label Nov 18, 2024
@mariusandra mariusandra removed the stale label Nov 26, 2024
@mariusandra mariusandra removed the stale label Dec 4, 2024
@PostHog PostHog deleted a comment from posthog-bot Dec 4, 2024
@PostHog PostHog deleted a comment from posthog-bot Dec 4, 2024
@PostHog PostHog deleted a comment from posthog-bot Dec 4, 2024
@PostHog PostHog deleted a comment from posthog-bot Dec 4, 2024
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@mariusandra mariusandra reopened this Jan 6, 2025
@posthog-bot posthog-bot removed the stale label Jan 7, 2025
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