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

Composite primary keys? #135

Closed
rightfold opened this Issue Dec 15, 2013 · 28 comments

Comments

Projects
None yet
7 participants
@rightfold

rightfold commented Dec 15, 2013

Are composite primary keys supported? If not, are there any technical reasons for this? Support for composite primary keys would be very useful especially when interfacing with existing databases.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 15, 2013

Member

We haven't added support for those yet. If you feel like you'd need it, the best way is to contribute it. :) Let us know if you need any help or you would like to send a proposal. :)

Member

josevalim commented Dec 15, 2013

We haven't added support for those yet. If you feel like you'd need it, the best way is to contribute it. :) Let us know if you need any help or you would like to send a proposal. :)

@josevalim josevalim closed this Dec 15, 2013

@qur2

This comment has been minimized.

Show comment
Hide comment
@qur2

qur2 Jan 22, 2014

+1/subscribing.

qur2 commented Jan 22, 2014

+1/subscribing.

@hamiltop

This comment has been minimized.

Show comment
Hide comment
@hamiltop

hamiltop Feb 2, 2015

I'm going to resurrect this and work on it.

My goal is to support:

create table(:bar_history, primary_key: [:bar_id, :date]) do
  add :bar_id, :integer
  add :date, :datetime
end

and

defmodule BarHistory do
  use Ecto.Model
  @primary_key [{:bar_id, :integer, []},{:date, :date, []}]

  schema "bar_history" do
    field :bar_id, :integer
    field :date, :date
  end
end

I should be able to pattern match on the array to detect the compound key while maintaining the current API.

Any known pitfalls I may encounter?

hamiltop commented Feb 2, 2015

I'm going to resurrect this and work on it.

My goal is to support:

create table(:bar_history, primary_key: [:bar_id, :date]) do
  add :bar_id, :integer
  add :date, :datetime
end

and

defmodule BarHistory do
  use Ecto.Model
  @primary_key [{:bar_id, :integer, []},{:date, :date, []}]

  schema "bar_history" do
    field :bar_id, :integer
    field :date, :date
  end
end

I should be able to pattern match on the array to detect the compound key while maintaining the current API.

Any known pitfalls I may encounter?

@hamiltop

This comment has been minimized.

Show comment
Hide comment
@hamiltop

hamiltop Feb 2, 2015

Another glance though and it looks like I don't want:

create table(:bar_history, primary_key: [:bar_id, :date]) do
  add :bar_id, :integer
  add :date, :datetime
end

in the migration. Instead, I want:

create table(:bar_history, primary_key: false) do
  add :bar_id, :integer
  add :date, :datetime
end

create index(:bar_history, [:bar_id, :date], unique: true, primary_key: true)

or something along those lines.

hamiltop commented Feb 2, 2015

Another glance though and it looks like I don't want:

create table(:bar_history, primary_key: [:bar_id, :date]) do
  add :bar_id, :integer
  add :date, :datetime
end

in the migration. Instead, I want:

create table(:bar_history, primary_key: false) do
  add :bar_id, :integer
  add :date, :datetime
end

create index(:bar_history, [:bar_id, :date], unique: true, primary_key: true)

or something along those lines.

@hamiltop

This comment has been minimized.

Show comment
Hide comment
@hamiltop

hamiltop Feb 2, 2015

Proof of concept at: https://github.com/hamiltop/ecto/tree/compound_primary_keys

Needs tests and some cleanup but otherwise pretty functional.

hamiltop commented Feb 2, 2015

Proof of concept at: https://github.com/hamiltop/ecto/tree/compound_primary_keys

Needs tests and some cleanup but otherwise pretty functional.

@hamiltop

This comment has been minimized.

Show comment
Hide comment
@hamiltop

hamiltop Feb 3, 2015

@josevalim Care to weigh in on the approach I'm taking? I'd like to polish it up for a PR but I want to make sure I'm on a track that makes sense.

hamiltop commented Feb 3, 2015

@josevalim Care to weigh in on the approach I'm taking? I'd like to polish it up for a PR but I want to make sure I'm on a track that makes sense.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Feb 3, 2015

Member

Yup, this is on my queue. Travelling messed up my schedule this week. :)

Member

josevalim commented Feb 3, 2015

Yup, this is on my queue. Travelling messed up my schedule this week. :)

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Feb 5, 2015

Member

@hamiltop if we are going in this direction, I would make Ecto.Model.primary_key always return a list (empty list for no key). It will clean up the code and ensure we don't forget to handle composite keys in some places.

Member

josevalim commented Feb 5, 2015

@hamiltop if we are going in this direction, I would make Ecto.Model.primary_key always return a list (empty list for no key). It will clean up the code and ensure we don't forget to handle composite keys in some places.

@hamiltop

This comment has been minimized.

Show comment
Hide comment
@hamiltop

hamiltop Feb 5, 2015

Cool. I'll update my branch to do that and add some tests. One question:
Should we treat primary keys as an index and use create index or should
we maybe do create primary_key instead?

On Thu Feb 05 2015 at 6:18:33 AM José Valim notifications@github.com
wrote:

@hamiltop https://github.com/hamiltop if we are going in this
direction, I would make Ecto.Model.primary_key always return a list (empty
list for no key). It will clean up the code and ensure we don't forget to
handle composite keys in some places.


Reply to this email directly or view it on GitHub
#135 (comment).

hamiltop commented Feb 5, 2015

Cool. I'll update my branch to do that and add some tests. One question:
Should we treat primary keys as an index and use create index or should
we maybe do create primary_key instead?

On Thu Feb 05 2015 at 6:18:33 AM José Valim notifications@github.com
wrote:

@hamiltop https://github.com/hamiltop if we are going in this
direction, I would make Ecto.Model.primary_key always return a list (empty
list for no key). It will clean up the code and ensure we don't forget to
handle composite keys in some places.


Reply to this email directly or view it on GitHub
#135 (comment).

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Feb 5, 2015

Member

I like the primary_key solution in your branch. Any downsides in it?

Member

josevalim commented Feb 5, 2015

I like the primary_key solution in your branch. Any downsides in it?

@hamiltop

This comment has been minimized.

Show comment
Hide comment
@hamiltop

hamiltop Feb 5, 2015

primary_key trumps unique as keyword args. It's a little weird to me that
you can specify both but unique will be ignored. It sort of makes sense
since primary_key implies unique, but the behavior of [unique: false, primary_key: true] is potentially confusing.
On Thu Feb 05 2015 at 7:31:43 AM José Valim notifications@github.com
wrote:

I like the primary_key solution in your branch. Any downsides in it?


Reply to this email directly or view it on GitHub
#135 (comment).

hamiltop commented Feb 5, 2015

primary_key trumps unique as keyword args. It's a little weird to me that
you can specify both but unique will be ignored. It sort of makes sense
since primary_key implies unique, but the behavior of [unique: false, primary_key: true] is potentially confusing.
On Thu Feb 05 2015 at 7:31:43 AM José Valim notifications@github.com
wrote:

I like the primary_key solution in your branch. Any downsides in it?


Reply to this email directly or view it on GitHub
#135 (comment).

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Feb 5, 2015

Member

@hamiltop maybe we can make it type: :unique or type: :primary_key?

Member

josevalim commented Feb 5, 2015

@hamiltop maybe we can make it type: :unique or type: :primary_key?

@hamiltop

This comment has been minimized.

Show comment
Hide comment
@hamiltop

hamiltop Feb 6, 2015

And a third option of nil for non unique. That would break backwards
compatibility. Is that a problem? I can support unique: true as well with
the long term plan to remove it. I'm not sure what the "break things"
policy is in ecto. I'll do both type: :unique and unique: true and we
can decide when to remove unique: true later.

On Thu Feb 05 2015 at 2:50:52 PM José Valim notifications@github.com
wrote:

@hamiltop https://github.com/hamiltop maybe we can make it type: :unique
or type: :primary_key?


Reply to this email directly or view it on GitHub
#135 (comment).

hamiltop commented Feb 6, 2015

And a third option of nil for non unique. That would break backwards
compatibility. Is that a problem? I can support unique: true as well with
the long term plan to remove it. I'm not sure what the "break things"
policy is in ecto. I'll do both type: :unique and unique: true and we
can decide when to remove unique: true later.

On Thu Feb 05 2015 at 2:50:52 PM José Valim notifications@github.com
wrote:

@hamiltop https://github.com/hamiltop maybe we can make it type: :unique
or type: :primary_key?


Reply to this email directly or view it on GitHub
#135 (comment).

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Feb 6, 2015

Member
Member

josevalim commented Feb 6, 2015

@rightfold

This comment has been minimized.

Show comment
Hide comment
@rightfold

rightfold commented Feb 6, 2015

Amazing!

@szymon-jez

This comment has been minimized.

Show comment
Hide comment
@szymon-jez

szymon-jez commented May 27, 2015

@hamiltop @josevalim Any plans with this?

@hamiltop

This comment has been minimized.

Show comment
Hide comment
@hamiltop

hamiltop May 27, 2015

I might be able to carve out some time in the next few weeks to polish up
what I've got. No guarantees though.

On Wed, May 27, 2015, 8:01 AM Szymon Jeż notifications@github.com wrote:

@hamiltop https://github.com/hamiltop @josevalim
https://github.com/josevalim Any plans with this?


Reply to this email directly or view it on GitHub
#135 (comment).

hamiltop commented May 27, 2015

I might be able to carve out some time in the next few weeks to polish up
what I've got. No guarantees though.

On Wed, May 27, 2015, 8:01 AM Szymon Jeż notifications@github.com wrote:

@hamiltop https://github.com/hamiltop @josevalim
https://github.com/josevalim Any plans with this?


Reply to this email directly or view it on GitHub
#135 (comment).

@dthul

This comment has been minimized.

Show comment
Hide comment
@dthul

dthul commented Jan 13, 2016

+1

@Tica2

This comment has been minimized.

Show comment
Hide comment
@Tica2

Tica2 Jan 14, 2016

Contributor

What status we have here?
@hamiltop @josevalim Can I continue this work?

Thanks!

Contributor

Tica2 commented Jan 14, 2016

What status we have here?
@hamiltop @josevalim Can I continue this work?

Thanks!

@hamiltop

This comment has been minimized.

Show comment
Hide comment
@hamiltop

hamiltop Jan 14, 2016

Go for it! I'm pretty sure any work I did would be quite stale now.

On Thu, Jan 14, 2016, 6:01 AM Petrica Clement Chiriac <
notifications@github.com> wrote:

What status we have here?
@hamiltop https://github.com/hamiltop @josevalim
https://github.com/josevalim Can I continue this work?

Thanks!


Reply to this email directly or view it on GitHub
#135 (comment).

hamiltop commented Jan 14, 2016

Go for it! I'm pretty sure any work I did would be quite stale now.

On Thu, Jan 14, 2016, 6:01 AM Petrica Clement Chiriac <
notifications@github.com> wrote:

What status we have here?
@hamiltop https://github.com/hamiltop @josevalim
https://github.com/josevalim Can I continue this work?

Thanks!


Reply to this email directly or view it on GitHub
#135 (comment).

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jan 14, 2016

Member

@Tica2 please go ahead. We already "merged" some of this, like making primary_key always return a list. :)

Member

josevalim commented Jan 14, 2016

@Tica2 please go ahead. We already "merged" some of this, like making primary_key always return a list. :)

@Tica2

This comment has been minimized.

Show comment
Hide comment
@Tica2

Tica2 Jan 14, 2016

Contributor

Migration

For migration we have:

defmodule HelloPhoenix.Repo.Migrations.CreatePlayer do
  use Ecto.Migration

  def change do
    create table(:players, primary_key: false) do
      add :name, :string, primary_key: true
      add :position, :string
      add :number, :integer

      timestamps
    end
  end
end

Is natural to support Composite primary keys with

defmodule HelloPhoenix.Repo.Migrations.CreatePlayer do
  use Ecto.Migration

  def change do
    create table(:players, primary_key: false) do
      add :name, :string, primary_key: true
      add :position, :string, primary_key: true
      add :number, :integer

      timestamps
    end
  end
end

Schema

For schema we have:

defmodule HelloPhoenix.Player do
  use HelloPhoenix.Web, :model

  @primary_key {:name, :string, []}
  schema "players" do
    field :position, :string
    field :number, :integer

    timestamps
  end
  . . .

I see 2 options

  1. Add atribute primary_key: true for field (Like Migrations)
defmodule HelloPhoenix.Player do
  use HelloPhoenix.Web, :model

  @primary_key false
  schema "players" do
    field :name, :string, primary_key: true
    field :position, :string, primary_key: true
    field :number, :integer

    timestamps
  end
  . . .
  1. extend @primary_key with list of 3-Tuple
defmodule HelloPhoenix.Player do
  use HelloPhoenix.Web, :model

  @primary_key [{:name, :string, []}, {:position, :string, []]
  schema "players" do
    field :number, :integer

    timestamps
  end
  . . .
  1. both

What to choose?

Contributor

Tica2 commented Jan 14, 2016

Migration

For migration we have:

defmodule HelloPhoenix.Repo.Migrations.CreatePlayer do
  use Ecto.Migration

  def change do
    create table(:players, primary_key: false) do
      add :name, :string, primary_key: true
      add :position, :string
      add :number, :integer

      timestamps
    end
  end
end

Is natural to support Composite primary keys with

defmodule HelloPhoenix.Repo.Migrations.CreatePlayer do
  use Ecto.Migration

  def change do
    create table(:players, primary_key: false) do
      add :name, :string, primary_key: true
      add :position, :string, primary_key: true
      add :number, :integer

      timestamps
    end
  end
end

Schema

For schema we have:

defmodule HelloPhoenix.Player do
  use HelloPhoenix.Web, :model

  @primary_key {:name, :string, []}
  schema "players" do
    field :position, :string
    field :number, :integer

    timestamps
  end
  . . .

I see 2 options

  1. Add atribute primary_key: true for field (Like Migrations)
defmodule HelloPhoenix.Player do
  use HelloPhoenix.Web, :model

  @primary_key false
  schema "players" do
    field :name, :string, primary_key: true
    field :position, :string, primary_key: true
    field :number, :integer

    timestamps
  end
  . . .
  1. extend @primary_key with list of 3-Tuple
defmodule HelloPhoenix.Player do
  use HelloPhoenix.Web, :model

  @primary_key [{:name, :string, []}, {:position, :string, []]
  schema "players" do
    field :number, :integer

    timestamps
  end
  . . .
  1. both

What to choose?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jan 15, 2016

Member

That's the hard question. :( I would propose the primary_key: true option in both cases. But we will need to get rid of @primary_key otherwise it will be confusing.

On the other hand, defining fields automatically is still useful. So we instead of @primary_key, we could introduce another option like @schema_fields. This option will declare fields to be used by default on all schemas. So instead of:

@primary_key {:id, :id, []}

One would write:

@schema_fields [{:id, :id, primary_key: true}]

You can also set it to an empty list to mean no default fields. Thoughts?

Member

josevalim commented Jan 15, 2016

That's the hard question. :( I would propose the primary_key: true option in both cases. But we will need to get rid of @primary_key otherwise it will be confusing.

On the other hand, defining fields automatically is still useful. So we instead of @primary_key, we could introduce another option like @schema_fields. This option will declare fields to be used by default on all schemas. So instead of:

@primary_key {:id, :id, []}

One would write:

@schema_fields [{:id, :id, primary_key: true}]

You can also set it to an empty list to mean no default fields. Thoughts?

@Tica2

This comment has been minimized.

Show comment
Hide comment
@Tica2

Tica2 Jan 15, 2016

Contributor

In migration we still have primary_key: false

create table(:players, primary_key: false) do
In this case id is not inserted automatically and you need to set by hand with primary_key: true or :string type ...

I prefer this method 1) with no changes for @primary_key:
default current
@primary_key {:id, :id, autogenerate: true}

manual construct string key, Composite primary keys
set @primary_key false like migration and put explicit in fields

defmodule HelloPhoenix.Player do
  use HelloPhoenix.Web, :model

  @primary_key false
  schema "players" do
    field :name, :string, primary_key: true
    field :position, :string, primary_key: true
    field :number, :integer

    timestamps
  end
  . . .

In this way we do not need any change to migrate to new Ecto Version.

Looks good for me and is like Migrations.

Thoughts?

Contributor

Tica2 commented Jan 15, 2016

In migration we still have primary_key: false

create table(:players, primary_key: false) do
In this case id is not inserted automatically and you need to set by hand with primary_key: true or :string type ...

I prefer this method 1) with no changes for @primary_key:
default current
@primary_key {:id, :id, autogenerate: true}

manual construct string key, Composite primary keys
set @primary_key false like migration and put explicit in fields

defmodule HelloPhoenix.Player do
  use HelloPhoenix.Web, :model

  @primary_key false
  schema "players" do
    field :name, :string, primary_key: true
    field :position, :string, primary_key: true
    field :number, :integer

    timestamps
  end
  . . .

In this way we do not need any change to migrate to new Ecto Version.

Looks good for me and is like Migrations.

Thoughts?

@Tica2

This comment has been minimized.

Show comment
Hide comment
@Tica2

Tica2 Jan 15, 2016

Contributor

@josevalim can we set this issue as open, to find in open list ?

Contributor

Tica2 commented Jan 15, 2016

@josevalim can we set this issue as open, to find in open list ?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jan 15, 2016

Member

I agree with what you said. One of the downsides is that "primary_key: true" on migration makes them a little bit more awkward on the adapter side. The adapter needs to traverse all of them to find out how to build the primary key. It can also make alter table cumbersome:

alter table(...) do
  add :foo, :string, primary_key: true
end 

Does it mean it is a whole new primary key? should we add it to the existing one?

I will reopen the issue BUT let's create a new one once we figure out how it should look like.

Member

josevalim commented Jan 15, 2016

I agree with what you said. One of the downsides is that "primary_key: true" on migration makes them a little bit more awkward on the adapter side. The adapter needs to traverse all of them to find out how to build the primary key. It can also make alter table cumbersome:

alter table(...) do
  add :foo, :string, primary_key: true
end 

Does it mean it is a whole new primary key? should we add it to the existing one?

I will reopen the issue BUT let's create a new one once we figure out how it should look like.

@josevalim josevalim reopened this Jan 15, 2016

@Tica2

This comment has been minimized.

Show comment
Hide comment
@Tica2

Tica2 Jan 15, 2016

Contributor

Will add :foo as PK, if first declaration table has PK database will throw exception.

BTW? But now how is working in migration?
I will check ...

Contributor

Tica2 commented Jan 15, 2016

Will add :foo as PK, if first declaration table has PK database will throw exception.

BTW? But now how is working in migration?
I will check ...

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Jan 22, 2016

Member

Closing in favor of the PR. :)

Member

josevalim commented Jan 22, 2016

Closing in favor of the PR. :)

@josevalim josevalim closed this Jan 22, 2016

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