-
Notifications
You must be signed in to change notification settings - Fork 1k
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
support to check if a column is primary key #744
Conversation
schema/schema.go
Outdated
func (ta *Table) GetPKColumn(index int) *TableColumn { | ||
return &ta.Columns[ta.PKColumns[index]] | ||
} | ||
|
||
// Get Index by column index. Return nil if the column is not a index. |
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.
Get Index by column index
What if the column has multiple indexes?
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.
Yes, I think this function is to get primary index only. PKColumns
must be PRIMARY
. I Will update.
schema/schema.go
Outdated
func (ta *Table) GetPKColumn(index int) *TableColumn { | ||
return &ta.Columns[ta.PKColumns[index]] | ||
} | ||
|
||
// Get Primary Index by column index. Return nil if the column is not a primary index. | ||
func (ta *Table) GetPKIndex(colIndex int) *Index { |
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.
I'm not sure what's your purpose. If you want to get PK, you can add a new field just like Table.PKColumns
Also please change schema_test.go to add a test covering your change
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.
This function is very useful when you want to check if a column is a primary key. For example, when you get a canal.RowsEvent
, and you may want to find out the primary key in the row. However, what we have currently is just Table.PKColumns
, which is a slice of column index for the table's primary key.
Line 423 in 0999529
ta.PKColumns[i] = ta.FindColumn(pkCol) |
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.
The usage of this function "Get Primary Index by column index." is very weird, primary key is a property of table, column is a property of table or index, it's strange to do reverse thing. I'm considering to return a bool to indicate if a column is in a primary key, but that's too trivial to add a public function, one line slices.Contains(Table.PKColumns, colIndex)
should be enough.
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.
Yes I agree with that it is enough to return a boolean, named to IsPrimaryKey()
. And the existing GetPKColumn()
is also weird, haha.
And slices.Contains
requires an extra import which is not necessary if you just need this simple function.
I also added unit test for the existing GetPKColumn()
.
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.
rest lgtm
schema/schema.go
Outdated
@@ -183,10 +183,23 @@ func (ta *Table) FindColumn(name string) int { | |||
return -1 | |||
} | |||
|
|||
// Get TableColumn by primary index index |
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.
// Get TableColumn by primary index index | |
// Get TableColumn by column index of primary key. |
schema/schema.go
Outdated
return &ta.Columns[ta.PKColumns[index]] | ||
} | ||
|
||
func (ta *Table) IsPrimaryKey(colIndex int) bool { | ||
for _, _colIndex := range ta.PKColumns { |
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.
maybe you can just use i
instead of _colIndex
, we don't have _xxx
stype in other codes.
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.
done
No description provided.