Skip to content

Support tuples in filter expressions#2344

Merged
josevalim merged 3 commits into
elixir-ecto:masterfrom
JakeBecker:tuples
Dec 15, 2017
Merged

Support tuples in filter expressions#2344
josevalim merged 3 commits into
elixir-ecto:masterfrom
JakeBecker:tuples

Conversation

@JakeBecker

@JakeBecker JakeBecker commented Dec 12, 2017

Copy link
Copy Markdown
Contributor

MySQL and Postgres support tuple comparisons, as discussed in this mailing list thread: https://groups.google.com/forum/#!topic/elixir-ecto/M5P7RYHg5FU

I've taken a stab at adding support for tuple syntax in queries, so you can do things like where([a], {a.x, a.y} > {^foo,^bar}). It should handle casting parameters correctly.

One thing I was not able to do was to make it accept parameters that are tuples. I think it'd be nice if it could do this, so you could do something like {a.x,a.y} > ^cursor, but I found the type system in Postgrex particularly hard to make sense of. I could use some help from someone more experienced with it to make that work.

Please review. Thanks!

@sourcelevel-bot

Copy link
Copy Markdown

Hello, @JakeBecker! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@josevalim

josevalim commented Dec 12, 2017

Copy link
Copy Markdown
Member

One thing I was not able to do was to make it accept parameters that are tuples. I think it'd be nice if it could do this, so you could do something like {a.x,a.y} > ^cursor

Would that be actually valid in Postgres?

Or, to go a bit deeper, how would we encode tuples in Postgrex? /cc @ericmj

Comment thread lib/ecto/type.ex Outdated
:utc_datetime | :naive_datetime | :date | :time | :any |
:utc_datetime_usec | :naive_datetime_usec | :time_usec
@typep composite :: {:array, t} | {:map, t} | {:embed, Ecto.Embedded.t} | {:in, t}
@typep composite :: {:array, t} | {:map, t} | {:embed, Ecto.Embedded.t} | {:in, t} | {:tuple, [t]}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably handle this in the planner as we don't want to expose the tuple type to users. For example, we don't want to allow field :foo, {:tuple, [:integer, :integer]}.

@JakeBecker JakeBecker Dec 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is this different from :in? I assumed that in was just for x.some_field in ^arr, and that tuple could behave similarly in that neither should be used for an actual field type.

@ericmj

ericmj commented Dec 12, 2017

Copy link
Copy Markdown
Member

Or, to go a bit deeper, how would we encode tuples in Postgrex?

In postgrex we can decode anonymous composite types to tuples because we have type information for each element in the composite. We cannot encode anonymous composites because when encoding we dont have type information for each element.

If the composite type is named, for example a table type, we can encode it but you would have to be explicit with a type cast because postgres can usually not infer composite types.

So in short, no we cant.

@JakeBecker

JakeBecker commented Dec 13, 2017

Copy link
Copy Markdown
Contributor Author

I'm still fuzzy on the type system in Postgrex and on Postgres's binary protocol. Is a tuple significantly different from other parameters? In either case, we can often determine at compile time the type. In where([x], {x.foo, x.bar} > {^a, ^b}), it figures out that a and b correspond to x.foo and x.bar and should have the same types. That should work if we do where([x], {x.foo, x.bar} > ^cursor) too, I would have thought.

Is an anonymous tuple possible to encode like this? I took a brief stab at this but I was stymied by the type system with oids and extensions and all.

@JakeBecker

Copy link
Copy Markdown
Contributor Author

Okay, I think I get it. Postgrex gets a ParameterDescription message back with an oid of 2249, which simply indicates a composite type with no indication of what types the elements should have. That makes things more difficult...

@josevalim

Copy link
Copy Markdown
Member

@JakeBecker How does it work for MySQL? It just works?

@JakeBecker

Copy link
Copy Markdown
Contributor Author

Nope, doesn't seem to work there either. When I try to execute a query with an expression like where([x], {x.foo, x.bar} > ^cursor) and cursor = {"a", "b"}, I get this error:

** (Mariaex.Error) (1241): Operand should contain 2 column(s)
    (ecto) lib/ecto/adapters/sql.ex:428: Ecto.Adapters.SQL.execute_and_cache/6
    (ecto) lib/ecto/adapters/sql.ex:402: Ecto.Adapters.SQL.execute/5
    (ecto) lib/ecto/repo/queryable.ex:133: Ecto.Repo.Queryable.execute/5
    (ecto) lib/ecto/repo/queryable.ex:37: Ecto.Repo.Queryable.all/4

@josevalim

Copy link
Copy Markdown
Member

@JakeBecker so probably we just can't accept it, which I think makes this way less useful?

@JakeBecker

JakeBecker commented Dec 13, 2017

Copy link
Copy Markdown
Contributor Author

I notice that if you do an expression like from u in User, where: u.email in ^x and x = ["a", "b"] Mariaex automatically turns that into SELECT ... FROM users AS u0 WHERE (u0.email IN (?,?)) ["a", "b"]. It expands the variable into a tuple value and adds ?s for each element.

Something like that might work for tuples too. I'm not sure where that transformation happens though.

@josevalim

Copy link
Copy Markdown
Member

@JakeBecker then I would rather force developers to use {^foo, ^bar}.

@josevalim

Copy link
Copy Markdown
Member

Using {^foo, ^bar} would also solve the issue with casting. We would never need to cast to a tuple because it exists only in the query syntax.

@JakeBecker

Copy link
Copy Markdown
Contributor Author

Okay, I undid those changes.

@josevalim

Copy link
Copy Markdown
Member

This looks great @JakeBecker! Can we have a test in filter_test or in planner_test that shows we raise a reasonably good message if someone tries where: {c1.foo, c2.bar} > ^var? Other than that, this looks ready to go!

@josevalim josevalim merged commit f64a650 into elixir-ecto:master Dec 15, 2017
@josevalim

Copy link
Copy Markdown
Member

❤️ 💚 💙 💛 💜

bartekupartek pushed a commit to bartekupartek/ecto that referenced this pull request Mar 19, 2019
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