-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: DESCRIBE does not show primary key #6068
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
Conversation
Thanks for the PR, could you add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
This is changing a bit too much in a non-backwards compatible manner. Instead of changing PRAGMA table_info
- perhaps we can rewrite PragmaShow
to use the other catalog functions instead. For example, we can use the duckdb_constraints
and duckdb_columns
functions which already have this information:
CREATE TABLE t2 (id INTEGER PRIMARY KEY, j VARCHAR UNIQUE);
SELECT
column_name,
data_type AS "column_type",
CASE WHEN is_nullable THEN 'YES' ELSE 'NO' END AS "null",
(
SELECT MIN(
CASE WHEN constraint_type='PRIMARY KEY' THEN 'PRI'
WHEN constraint_type='UNIQUE' THEN 'UNI'
ELSE NULL END)
FROM duckdb_constraints() c
WHERE c.table_oid=cols.table_oid AND list_contains(constraint_column_names, cols.column_name)
) AS "key",
column_default AS "default",
NULL AS "extra"
FROM duckdb_columns cols WHERE table_name='t2';
┌─────────────┬─────────────┬─────────┬─────────┬─────────┬───────┐
│ column_name │ column_type │ null │ key │ default │ extra │
│ varchar │ varchar │ varchar │ varchar │ varchar │ int32 │
├─────────────┼─────────────┼─────────┼─────────┼─────────┼───────┤
│ id │ INTEGER │ NO │ PRI │ NULL │ NULL │
│ j │ VARCHAR │ YES │ UNI │ NULL │ NULL │
└─────────────┴─────────────┴─────────┴─────────┴─────────┴───────┘
This way the other tests can be left as-is and we really only change the output of SHOW
to correctly show unique constraints/primary keys.
b6c57c3
to
355d543
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! Looks good - some more comments.
Also there is no need to continuously merge with master and push. It is re-triggering the CI unnecessarily and taking up CI time on the main repo. |
296d9a7
to
2f8514d
Compare
- Change PragmaShow macro - Add and update test cases
Seems like test that are failing are not failing on my changes :/ |
Please stop merging with master to retrigger CI. I have asked you this before. It is consuming shared CI resources. If there is an unrelated CI failure due to a network outage you can state that. You can also look at other PRs and see if they have the same problem. |
Sorry for that, I just merged most recent changes to my fork |
Thanks for the updates - looks good to me. Failures are indeed unrelated |
fixes #1468