Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6343 : Phoenix allows duplicate column names when one of them is a primary key - #1118
Conversation
| assertTrue(splits.size() > 0); | ||
| } | ||
| @Test |
There was a problem hiding this comment.
Can you add a test for adding the identical column with an alter table statement ?
There was a problem hiding this comment.
Done in the same test
stoty
commented
Jan 28, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 28, 2021
💔 -1 overall
This message was automatically generated. |
Shall we disable allowing same column name as primary key under any columnFamily or should we just disable it under default columnFamily? |
stoty
commented
Jan 28, 2021
Good question.
This should be easy enough to check with hbase shell. |
virajjasani
commented
Jan 28, 2021
Nice points.
For user created tables, their pk (and non-pk) columns are actually rowkeys in SYSTEM.CATALOG. The cells represent specific properties like
I think we can't specify CF for PK. We have this special Exception for it: Based on some unit tests written very initially, it seems we do allow creating same column name (with user defined columnFamily) as Pk column name, but that is the very reason why the same would apply to default columnFamily and hence, any user created table can have Pk and non-Pk column names same. |
stoty
commented
Jan 28, 2021
Thanks for checking. In this case, IMO we should handle this like the standard columns. If we allow the same name in different column families, then we should only forbid matching names in the default CF. |
virajjasani
commented
Jan 28, 2021
It seems from visibility viewpoint, the problem remains same. Even with user defined CF, we see same column name as pk column. |
stoty
commented
Jan 28, 2021
That is more of an sqlline/presentation problem. Based on this, we should only disallow the PK names in the default CF. |
stoty
commented
Jan 28, 2021
i.e you can get rs.getString("cfa.a"), and rs.getString("cfb.a"), but if you cannot get do the same for PK.a vs default cf.a . |
virajjasani
commented
Jan 28, 2021
Right, mostly it is representation problem. You are right, we should stick to default CF. |
gjacoby126
left a comment
There was a problem hiding this comment.
One nit, otherwise lgtm
| String viewName = generateUniqueName(); | ||
| try (Connection conn = DriverManager.getConnection(getUrl(), props)) { | ||
| try { | ||
| conn.createStatement().execute(String.format("CREATE TABLE %s" |
There was a problem hiding this comment.
nit: Would be good if before each case there was a brief comment explaining it -- e.g "Same column name, different column type" for the third one.
gjacoby126
left a comment
There was a problem hiding this comment.
+1, assuming tests pass. Thanks @virajjasani
stoty
commented
Jan 28, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 28, 2021
💔 -1 overall
This message was automatically generated. |
virajjasani
commented
Jan 28, 2021
Latest changes include some minor changes in |
stoty
commented
Jan 28, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 28, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 10, 2021
💔 -1 overall
This message was automatically generated. |
… is a primary key (#1118) Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org>
No description provided.