Reporting unique nulls not distinct in analyze schema and assessment report #2125
Conversation
ShivanshGahlot
requested review from
priyanshi-yb,
sanyamsinghal and
makalaaneesh
| ); | ||
|
|
||
| ALTER TABLE sales_unique_nulls_not_distinct_alter | ||
| ADD CONSTRAINT sales_unique_nulls_not_distinct_alter_unique UNIQUE NULLS NOT DISTINCT (store_id, product_id, sale_date); |
There was a problem hiding this comment.
This can also come up as a CREATE UNIQUE INDEX not just constraints.. https://www.postgresql.org/docs/current/indexes-unique.html
I think we should check for that as well? cc: @priyanshi-yb
There was a problem hiding this comment.
I have added check for CREATE UNIQUE INDEX too and am using struct now.
ShivanshGahlot
force-pushed
the
shivansh/unique-nulls
branch
from
4014f2d to
e59cd44
Compare
ShivanshGahlot
force-pushed
the
shivansh/unique-nulls
branch
from
34668fd to
71ad160
Compare
| if indexStmt.Unique && indexStmt.NullsNotDistinct { | ||
| d.detected = true | ||
| } | ||
| } else if queryparser.GetMsgFullName(msg) == queryparser.PG_QUERY_TABLECONSTRAINT_NODE { |
There was a problem hiding this comment.
Will this node come up for both ALTER TABLE ADD CONSTRAINT as well as CREATE TABLE ?
There was a problem hiding this comment.
nice. I was initially going to suggest we write this code inside tableIssueDetector and alterTableIssueDetector, but this is useful, since we just have to find the constraint node and check.
cc: @priyanshi-yb
| // Detect checks if a unique constraint is defined which has nulls not distinct | ||
| func (d *UniqueNullsNotDistinctDetector) Detect(msg protoreflect.Message) error { | ||
| if queryparser.GetMsgFullName(msg) == queryparser.PG_QUERY_INDEX_STMT_NODE { | ||
| proto := msg.Interface().(proto.Message) |
There was a problem hiding this comment.
Can you define a method in queryparser similar to ProtoAsSelectStmt that gets the struct itself.
Also, when you are doing type assertions, always prefer using x, ok := syntax to avoid panics. if !ok, return an error
There was a problem hiding this comment.
Haven't created a function for the PG_QUERY_TABLECONSTRAINT_NODE code below since it doesn't seem like a regularly used pattern. Do you think otherwise?
There was a problem hiding this comment.
I think the goal here is to keep most of the pure parser/proto related code in queryparser pkg. So even if not very re-usable, would recommend defining a function there. We also avoid importing and directly using pg_query pkg in queryissue package that way.
| return fmt.Errorf("failed to cast msg to %s", queryparser.PG_QUERY_TABLECONSTRAINT_NODE) | ||
| } | ||
|
|
||
| if constraintNode.Contype == pg_query.ConstrType_CONSTR_UNIQUE && constraintNode.NullsNotDistinct { |
There was a problem hiding this comment.
define pg_query.ConstrType_CONSTR_UNIQUE in queryparser pkg.
Again, high level goal is to not import pg_query pkg in queryissue as much as possible; keep all the code related to parser inside queryparser pkg.
| indexStmt, err := queryparser.ProtoAsIndexStmt(msg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if indexStmt.Unique && indexStmt.NullsNotDistinct { | ||
| d.detected = true | ||
| } |
There was a problem hiding this comment.
I don't see a need to convert here, and add a overhead.
unique and nullsnotdistinct are directly present in the protomsg as fields.
You can directly use GetBoolField(msg, fieldName) to get that.
There was a problem hiding this comment.
Hey @sanyamsinghal I am a little confused. So according to yesterday's discussion what should I do? Keep it as a struct implementation since it is inside the Detect() function?
There was a problem hiding this comment.
Yeah you can keep it as it is
| constraintNode, err := queryparser.ProtoAsTableConstraint(msg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if constraintNode.Contype == queryparser.UNIQUE_CONSTR_TYPE && constraintNode.NullsNotDistinct { | ||
| d.detected = true | ||
| } |
There was a problem hiding this comment.
not sure about this. but same might apply here.
There was a problem hiding this comment.
I had earlier implemented it using the protomsg approach. I had to use a new function to convert the enum number of Contype to the name of that contype like UNIQUE_CONSTR_TYPE to make it more readable and not susceptible to any changes in the enum numbers map.
But the current approach seemed more direct and easier to read.
What do you think should be done here?
| @@ -52,6 +52,9 @@ const ( | |||
| PG_QUERY_JSON_IS_PREDICATE_NODE = "pg_query.JsonIsPredicate" | |||
| PG_QUERY_VIEWSTMT_NODE = "pg_query.ViewStmt" | |||
| PG_QUERY_COPYSTSMT_NODE = "pg_query.CopyStmt" | |||
|
|
|||
| PG_QUERY_TABLECONSTRAINT_NODE = "pg_query.Constraint" | |||
| PG_QUERY_INDEX_STMT_NODE = "pg_query.IndexStmt" | |||
There was a problem hiding this comment.
nit: PG_QUERY_INDEXSTMT_NODE
| @@ -52,6 +52,9 @@ const ( | |||
| PG_QUERY_JSON_IS_PREDICATE_NODE = "pg_query.JsonIsPredicate" | |||
| PG_QUERY_VIEWSTMT_NODE = "pg_query.ViewStmt" | |||
| PG_QUERY_COPYSTSMT_NODE = "pg_query.CopyStmt" | |||
|
|
|||
| PG_QUERY_TABLECONSTRAINT_NODE = "pg_query.Constraint" | |||
There was a problem hiding this comment.
lets keep the constant name same as node name - PG_QUERY_CONSTRAINT_NODE ?
There was a problem hiding this comment.
Yeah makes sense. Done.
ShivanshGahlot
force-pushed
the
shivansh/unique-nulls
branch
from
94bd4c8 to
2e0ee2e
Compare
Describe the changes in this pull request
Reporting UNIQUE NULLS NOT DISTINCT feature in CREATE and ALTER TABLE as unsupported in the analyze schema report and migration assessment report.
Issue:
#1978
Describe if there are any user-facing changes
Changes in assessment report:
Changes in schema report:
How was this pull request tested?
Tested locally
Also added unit tests and end to end tests
Does your PR have changes that can cause upgrade issues?