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

Support tuples #611

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Support tuples #611

merged 4 commits into from
Jul 25, 2023

Conversation

koskimas
Copy link
Member

This PR adds tuple support. After trying out a bunch of options, I ended up with a function overload approach up to tuples of length five. There's also a partially unsafe $asTuple function in SelectQueryBuilder that I don't love.

The reason for my choices basically boils down to the fact that typescript doesn't allow the index of a type in an union to be observed. They've stated in some issues that it will never be observable.

Because of this, there's no (sane) conversion from an object to a tuple, which is why the $asTuple method is necessary. If you google this, there is a solution, but it takes around 3 gigabytes of memory to convert a single object to a tuple, so that's a hard no-no.

Using function overloads instead of a recursive type (like the one in coalesce) is also due to performance AND autocompletion issues. The recursive type immediately caused ~one second delays and I wasn't able to get autocompletion to work. Just like we don't currently have autocompletion in coalesce after the first argument.

Having a maximum length of five for tuples shouldn't be an issue. My guess is that the most common use case for this is composite primary keys and the length for those is usually 2 or 3. We can also add more overloads if someone requests them.

Closes #367

@koskimas koskimas requested a review from igalklebanov July 23, 2023 07:21
@vercel
Copy link

vercel bot commented Jul 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 25, 2023 11:30am

@koskimas koskimas changed the title Add tuples in expression builder Support tuples Jul 23, 2023
@koskimas koskimas force-pushed the add-tuples-in-expression-builder branch from e615446 to 14fdc8d Compare July 23, 2023 07:44
@igalklebanov igalklebanov added enhancement New feature or request api Related to library's API labels Jul 23, 2023
@thelinuxlich
Copy link
Contributor

Wouldn't refTuple be a better option, following the pattern of whereRef?

@koskimas
Copy link
Member Author

koskimas commented Jul 24, 2023

Wouldn't refTuple be a better option, following the pattern of whereRef?

Yeah, but on the other hand, most functions take references by default. I don't know which is the better "default". @igalklebanov Thoughts?

@koskimas koskimas force-pushed the add-tuples-in-expression-builder branch from 14fdc8d to b782a0d Compare July 24, 2023 20:15
@koskimas
Copy link
Member Author

koskimas commented Jul 24, 2023

@thelinuxlich Did you mean we'd have refTuple and valTuple or refTuple and tuple?

@thelinuxlich
Copy link
Contributor

refTuple and tuple

@thelinuxlich
Copy link
Contributor

because val at least there is no precedent in the API, right?

@igalklebanov
Copy link
Member

igalklebanov commented Jul 24, 2023

@koskimas @thelinuxlich
Sorry, I didn't go over the changes yet, but regarding the naming..
feels like it should be tuple(...literalsOrExpressions) and tupleRef(...refs).
it'll be mostly used with literals in where clauses, and this way it's aligned with other places in the API - consistency.

@igalklebanov
Copy link
Member

igalklebanov commented Jul 24, 2023

@koskimas @thelinuxlich Sorry, I didn't go over the changes yet, but regarding the naming.. feels like it should be tuple(...literalsOrExpressions) and tupleRef(...refs). it'll be mostly used with literals in where clauses, and this way it's aligned with other places in the API - consistency.

Actually, when used in where, there's still going to be ref usage in the lhs. This is a tough one, but seems like consistency should rule here.

@koskimas
Copy link
Member Author

koskimas commented Jul 25, 2023

I think it should be refTuple "reference tuple" instead of tupleRef "tuple reference". Tuple reference sounds like a reference to a tuple.

@koskimas
Copy link
Member Author

Actually, when used in where, there's still going to be ref usage in the lhs. This is a tough one, but seems like consistency should rule here.

Yeah, for now pretty much the only use case for this is

eb(refTuple('first_name', 'last_name'), '=', tuple('Jennifer', 'Aniston'))

so I'd say refTuple will be used just as often as tuple.

@koskimas
Copy link
Member Author

@igalklebanov @thelinuxlich I renamed the functions.

igalklebanov

This comment was marked as outdated.

Copy link
Member

@igalklebanov igalklebanov left a comment

Choose a reason for hiding this comment

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

Amazing stuff! 👨‍🎨

Left some comments..

src/parser/select-parser.ts Show resolved Hide resolved
src/query-builder/select-query-builder.ts Outdated Show resolved Hide resolved
@igalklebanov igalklebanov added mysql Related to MySQL sqlite Related to sqlite postgres Related to PostgreSQL labels Jul 25, 2023
@koskimas koskimas merged commit 53d9125 into master Jul 25, 2023
@koskimas koskimas deleted the add-tuples-in-expression-builder branch July 25, 2023 11:36
Gaspero pushed a commit to Gaspero/kysely that referenced this pull request Oct 2, 2023
* further type speedup and simplification

* Add tuple support

* rename tuple to refTuple and valTuple to tuple

* improve  typings
@panusoi panusoi mentioned this pull request Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to library's API enhancement New feature or request mysql Related to MySQL postgres Related to PostgreSQL sqlite Related to sqlite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

supporting "where in" with multiple columns
3 participants