Skip to content

Conversation

gkaretka
Copy link
Contributor

@gkaretka gkaretka commented Feb 1, 2023

fixes #1468

@gkaretka
Copy link
Contributor Author

gkaretka commented Feb 1, 2023

#1468

@Tishj
Copy link
Contributor

Tishj commented Feb 2, 2023

Thanks for the PR, could you add fixes #1468 to the description of the PR? That way it will automatically close the assigned issue :)

Copy link
Collaborator

@Mytherin Mytherin left a 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 │
│   varcharvarcharvarcharvarcharvarchar │ int32 │
├─────────────┼─────────────┼─────────┼─────────┼─────────┼───────┤
│ id          │ INTEGER     │ NO      │ PRI     │ NULLNULL │
│ j           │ VARCHAR     │ YES     │ UNI     │ NULLNULL │
└─────────────┴─────────────┴─────────┴─────────┴─────────┴───────┘

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.

Copy link
Collaborator

@Mytherin Mytherin left a 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.

@Mytherin
Copy link
Collaborator

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.

@gkaretka gkaretka force-pushed the master branch 2 times, most recently from 296d9a7 to 2f8514d Compare February 13, 2023 20:54
- Change PragmaShow macro
- Add and update test cases
@gkaretka
Copy link
Contributor Author

Seems like test that are failing are not failing on my changes :/

@Mytherin
Copy link
Collaborator

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.

@gkaretka
Copy link
Contributor Author

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

@Mytherin Mytherin merged commit 8a8581e into duckdb:master Feb 15, 2023
@Mytherin
Copy link
Collaborator

Mytherin commented Feb 15, 2023

Thanks for the updates - looks good to me. Failures are indeed unrelated

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.

DESCRIBE does not show primary key
3 participants