feat: support primary key alternate syntax

[parkma99]

Which issue does this PR close?

Closes #7152

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Yes , add a test case in datafusion/sql/tests/sql_integration.rs

Are there any user-facing changes?

[jonahgao]

Perhaps we should consider scenarios where primary keys are defined multiple times.

For example:

psql=> create table foo(
        a int primary key, 
        b int, 
        c int, 
        primary key(b,c)
);

ERROR:  multiple primary keys for table "foo" are not allowed
LINE 5:         primary key(b,c)

It will fail in PostgreSQL.

[parkma99]

Thanks @jonahgao , I will update this scenarios recently.

[mustafasrepo]

Perhaps we should consider scenarios where primary keys are defined multiple times.

For example:

psql=> create table foo(
        a int primary key, 
        b int, 
        c int, 
        primary key(b,c)
);

ERROR:  multiple primary keys for table "foo" are not allowed
LINE 5:         primary key(b,c)

It will fail in PostgreSQL.

I think we may choose not to fail, for these cases as long as both a and (b, c) satisfies primary key constraint.

[parkma99]

cc @mustafasrepo @jonahgao @alamb

[alamb]

👍 looks good to me -- thank you @parkma99 (for this and all your other contributions)

I had one suggestion about adding a "not implemented" error (and maybe some tests that cover the unsupported syntax) but since this PR is better than what is on master I think it could also be merged as is

[alamb]

Can you please add code here that returns a Err(DataFusionError::NotImplemented) for other types of constraints -- I think this code will silently ignore them

Maybe it could be something like

match option.option {
  ast::ColumnOption::Unique { is_primary } => ... 
  option => return Err(DataFusionError::NotImplemented(format!("Constraint {} not implemented", option))),
}

[alamb]

However, given that the current code will also ignore such errors I don't think this change is required

[parkma99]

I think it should not return Error because of without any constraints is ok.

[alamb]

I am not sure this means a multi part primary key -- postgres errors on this syntax

postgres=# create table person (id int, name varchar primary key, primary key(id));
ERROR:  multiple primary keys for table "person" are not allowed
LINE 1: ...e table person (id int, name varchar primary key, primary ke...

I think the way this is supposed to be expressed is like

create table person (id int, name varchar,  primary key(name,  id));

[mustafasrepo]

I am not sure this means a multi part primary key -- postgres errors on this syntax

postgres=# create table person (id int, name varchar primary key, primary key(id));
ERROR:  multiple primary keys for table "person" are not allowed
LINE 1: ...e table person (id int, name varchar primary key, primary ke...

I think the way this is supposed to be expressed is like

create table person (id int, name varchar,  primary key(name,  id));

I guess it is better to follow postgres in this case. Postgres supports multiple unique constraints. Hence following query

create table person (id int, name string primary key, primary key(id))

can be written as

create table person (id int, name string unique not null, primary key(id))

to define unique constraint. For writing composite primary key one have to use following syntax

create table person (id int, name varchar,  primary key(name,  id));

[alamb]

@parkma99 is this comment still outstanding?

[parkma99]

Sorry, I am at home this week.

[alamb]

no worries and no pressure! I was just trying to understand what the state of this PR was -- it looks like you are aware of the feedback and just haven't had a chance to implement it. I wanted to make sure you weren't waiting on additional feedback

I'll mark it as draft so it doesn't appear on the list of PRs needing review

[alamb]

I still think two primary keys on the same column doesn't really make sense, but we can always make this check more strict as a follow on PR I think

[alamb]

What do you think about adding unique constraints as well

create table person (id int unique, name string)

[parkma99]

I add a test case about unique constraints.

[alamb]

Marking as draft per #7160 (comment)

[alamb]

This PR looks good to me now; What do you think @mustafasrepo ?

[alamb]

I still think two primary keys on the same column doesn't really make sense, but we can always make this check more strict as a follow on PR I think

[mustafasrepo]

Sorry for the late response. I was at vacation, and didn't have time to look. Thanks @parkma99 for this PR. Thanks @alamb for the review.