SQL: extract types in "create type t"#4383
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4383 +/- ##
==========================================
+ Coverage 85.91% 85.93% +0.02%
==========================================
Files 252 252
Lines 62683 62715 +32
==========================================
+ Hits 53854 53895 +41
+ Misses 8829 8820 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds first-class tagging support for SQL CREATE TYPE statements so type names (e.g., t in CREATE TYPE t ...) can be extracted by the SQL parser, with accompanying unit tests and release notes.
Changes:
- Introduce a new SQL kind
type(k) and emit it fromparseType. - Suppress the
typetag when a more specific kind (record,table,cursor) is emitted for the same name. - Add/adjust unit tests covering PostgreSQL
CREATE TYPEvariants and update news.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| parsers/sql.c | Adds SQLTAG_TYPE kind and updates parseType to emit/suppress type tags appropriately. |
| docs/news/HEAD.rst | Documents the new SQL type kind extraction behavior. |
| Units/parser-sql.r/types.d/input.sql | New fixture with PostgreSQL CREATE TYPE examples. |
| Units/parser-sql.r/types.d/expected.tags | Expected tags for the new types.d test case, including k kind outputs. |
| Units/parser-sql.r/types.d/args.ctags | Test args for the new types.d suite. |
| Units/parser-sql.r/funcions.d/expected.tags | Updates expected tags to reflect record tagging behavior for create type ... as (...). |
| Units/parser-sql.r/funcions.d/args.ctags | Enables SQL record kind (+r) for the funcions.d test suite to match updated expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b180345 to
3046a12
Compare
3046a12 to
41a70dd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
41a70dd to
d39ebed
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
parsers/sql.c
Outdated
| markCorkEntryAsPlaceholder(r, true); | ||
| makeSqlTag (name, SQLTAG_RECORD); |
There was a problem hiding this comment.
In the KEYWORD_record/KEYWORD_object branch, the code always marks the newly-created SQLTAG_TYPE entry as a placeholder before attempting to emit SQLTAG_RECORD. If the record kind is disabled (SqlKinds['r'] is false by default), makeSqlTag(name, SQLTAG_RECORD) returns CORK_NIL and the placeholder call suppresses the only tag for this CREATE TYPE statement. Consider only calling markCorkEntryAsPlaceholder(r, true) when SQLTAG_RECORD is enabled / when makeSqlTag(...) actually created a tag, so a type tag is still emitted when record tags are disabled.
| markCorkEntryAsPlaceholder(r, true); | |
| makeSqlTag (name, SQLTAG_RECORD); | |
| if (isLanguageKindEnabled (Lang_sql, SQLTAG_RECORD)) | |
| { | |
| markCorkEntryAsPlaceholder(r, true); | |
| makeSqlTag (name, SQLTAG_RECORD); | |
| } |
parsers/sql.c
Outdated
| markCorkEntryAsPlaceholder(r, true); | ||
| makeSqlTag (name, SQLTAG_TABLE); | ||
| break; | ||
|
|
||
| case KEYWORD_ref: | ||
| readToken (token); | ||
| if (isKeyword (token, KEYWORD_cursor)) | ||
| { | ||
| markCorkEntryAsPlaceholder(r, true); | ||
| makeSqlTag (name, SQLTAG_CURSOR); |
There was a problem hiding this comment.
Similar to the record/object branch, the KEYWORD_table and KEYWORD_ref cursor branches unconditionally call markCorkEntryAsPlaceholder(r, true). If the alternative kind (table/cursor) is disabled via --kinds-SQL, this will suppress the type tag and leave the CREATE TYPE statement without any tag. Consider making the placeholder suppression conditional on successfully creating the table/cursor tag (or on isLanguageKindEnabled for that kind).
| markCorkEntryAsPlaceholder(r, true); | |
| makeSqlTag (name, SQLTAG_TABLE); | |
| break; | |
| case KEYWORD_ref: | |
| readToken (token); | |
| if (isKeyword (token, KEYWORD_cursor)) | |
| { | |
| markCorkEntryAsPlaceholder(r, true); | |
| makeSqlTag (name, SQLTAG_CURSOR); | |
| if (isLanguageKindEnabled (Lang_sql, SQLTAG_TABLE)) | |
| { | |
| /* When the table kind is enabled, emit a | |
| * table tag and suppress the type tag. */ | |
| markCorkEntryAsPlaceholder(r, true); | |
| makeSqlTag (name, SQLTAG_TABLE); | |
| } | |
| break; | |
| case KEYWORD_ref: | |
| readToken (token); | |
| if (isKeyword (token, KEYWORD_cursor)) | |
| { | |
| if (isLanguageKindEnabled (Lang_sql, SQLTAG_CURSOR)) | |
| { | |
| /* When the cursor kind is enabled, emit a | |
| * cursor tag and suppress the type tag. */ | |
| markCorkEntryAsPlaceholder(r, true); | |
| makeSqlTag (name, SQLTAG_CURSOR); | |
| } |
d39ebed to
0f95654
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
So units.py runs the unit test case and can report "FAILED (broken args.ctags?)" failure. Signed-off-by: Masatake YAMATO <yamato@redhat.com>
https://www.postgresql.jp/document/17/html/sql-createtype.html Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
0f95654 to
4b0e801
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Make the tag that OLDTAGINDEX specifies a placeholder, then make a new | ||
| * tag for TOKEN with KIND kind. */ | ||
| static int makeSqlTagInstead (tokenInfo *const token, const sqlKind kind, int oldTagIndex) | ||
| { | ||
| markCorkEntryAsPlaceholder(oldTagIndex, true); | ||
| return makeSqlTag (token, kind); | ||
| } |
There was a problem hiding this comment.
makeSqlTagInstead() only marks the original (leaf) cork entry as a placeholder. If XTAG_QUALIFIED_TAGS is enabled and the original tag had a non-empty scope, makeSqlTagFull() will also have emitted a separate qualified-tag entry; that qualified entry won’t be marked placeholder and can leak into the output even though the leaf tag was suppressed. Consider capturing the qualified-tag cork index when creating the original tag (use makeSqlTagFull(...,&fq)), and when replacing, mark both the leaf and qualified entries as placeholders (and likewise create the replacement tag via makeSqlTagFull so the replacement gets a qualified entry when applicable).
There was a problem hiding this comment.
Marking fq tags as placeholder is not needed in this pull request.
https://www.postgresql.jp/document/17/html/sql-createtype.html