Add VisibilityBitmap to TableMapEvent in replication

[dongwook-chan]

I was implementing optional_metadata field in table_map_event for python-mysql-replication.
And came across this repo and found that metadata type COLUMN_VISIBILITY isn't implemented.

Changes

• Added TableMapEvent.VisibilityBitmap
  • VisibilityBitmap is a bitmap where each bit represents the visibility of a corresponding column in a table.
  • If a bit is set, it indicates that the corresponding column is NOT an invinsible column.
  • Invisible column was introduced in MySQL version 8.0.23.
• TableMapEvent.VisibilityMap
  • VisibilityMap lists boolean values of which is true if column of the same index is NOT an invisible column.

Examples

You can check the visibility of a column by its table by executing DESCRIBE command.

mysql> SELECT VERSION(); -- must be MySQL 8.0.23 or higher. Not supported by MariaDB yet.
+-----------+
| VERSION() |
+-----------+
| 8.0.33-25 |
+-----------+
1 row in set (0.00 sec)

mysql> CREATE TABLE test_visibility (c1 INT, c2 INT INVISIBLE, c3 INT);
Query OK, 0 rows affected (0.03 sec)

mysql> DESC test_visibility;
+-------+------+------+-----+---------+-----------+
| Field | Type | Null | Key | Default | Extra     |
+-------+------+------+-----+---------+-----------+
| c1    | int  | YES  |     | NULL    |           |
| c2    | int  | YES  |     | NULL    | INVISIBLE |
| c3    | int  | YES  |     | NULL    |           |
+-------+------+------+-----+---------+-----------+
3 rows in set (0.00 sec)

[dongwook-chan]

@lance6716
I appreciate your positive reaction to my PR, and I'm grateful for your contributions to this repository. go-mysql is indispensable to me. Could you please advise whether I should include tests for this new addition?

[lance6716]

@lance6716 I appreciate your positive reaction to my PR, and I'm grateful for your contributions to this repository. go-mysql is indispensable to me. Could you please advise whether I should include tests for this new addition?

I will take a look some hours later. Thanks for your PR 😄

[dongwook-chan]

I added a co-author.

[atercattus]

nit: Maybe it's a little easier to understand the original code with countdown instead of 1<<uint(7-p%8)

[dongwook-chan]

@atercattus
I appreciate your review. Also, I agree with you and I can get this fixed.
Actually VisibilityMap is copy and paste of existing UnsignedMap. Should I fix UnsignedMap too?
I could create a separate PR it if it seems irrelevant to this PR.

[atercattus]

Oh. @lance6716 what do you think about this? Apply the Boy Scout Rule or not?

[dongwook-chan]

If we decide to improve, the code will look like below.

func (e *TableMapEvent) VisibilityMap() map[int]bool {
	if len(e.VisibilityBitmap) == 0 {
		return nil
	}
	ret := make(map[int]bool)
	for _, field := range e.VisibilityBitmap {
		for c, i := 0x80, 0; c != 0; c, i = c>>1, i+1 {
			ret[i] = field&byte(c) != 0
		}
	}
	return ret
}

[lance6716]

The old code is really not easy to understand 😢 I also hope we start to improve the readability. As for UnsignedMap, I'm OK that you also modify it in this PR or another PR. WDYT @atercattus

Your new code snippet seems not optimal. The status of i should be kept when iterate bytes of VisibilityBitmap. And we can early stop when the column count is not a multiple of 8 (though the original code seems not optimize about this). Maybe we can

...
i := 0
for e.VisibilityBitmap {
  for c := 0x80; ... {
    ret[i] = ...
    i++
    if i >= e.ColumnCount { return }
  }
}
...

Welcome to find different code

[dongwook-chan]

My bad. i should indeed be initialized outside of the inner loop.

[lance6716]

The old code is really not easy to understand 😢 I also hope we start to improve the readability. As for UnsignedMap, I'm OK that you also modify it in this PR or another PR. WDYT @atercattus

Your new code snippet seems not optimal. The status of i should be kept when iterate bytes of VisibilityBitmap. And we can early stop when the column count is not a multiple of 8 (though the original code seems not optimize about this). Maybe we can

...
i := 0
for e.VisibilityBitmap {
  for c := 0x80; ... {
    ret[i] = ...
    i++
    if i >= e.ColumnCount { return }
  }
}
...

Welcome to find different code

[lance6716]

As for testing, I think maybe you can capture/construct some VisibilityBitmap bytes and test the result of TableMapEvent.VisibilityMap is expected.

[dongwook-chan]

@lance6716
Sure. I think I should create a separate function. Just as TestTableMapOptMetaPrimaryKey is defined solely for primary key type meta data. At first, I thought about squeezing into TestTableMapOptMetaNames or TestTableMapHelperMaps, but it's too packed and I saw no room for any addition. Your advice would be help me a lot. 😀

[dongwook-chan]

I added test case for 8.0 only, because the keyword INVISIBLE is not available in any other vendors or versions.

[lance6716]

ptal @atercattus

[dongwook-chan]

But on a second thought, I would like to add case 2 where invisible columns do not exist at all! This case will be applicable to the rest of vendors and versions too; MySQL 5.7, MariaDB 10.4 and MairaDB 10.5.

[lance6716]

lgtm now. let's wait @atercattus

[lance6716]

@dongwook-chan seems atercattus is a bit busy. Do you want me to merge it or you can wait a bit longer ?

[dongwook-chan]

@lance6716 No need to wait any longer!
@atercattus Thank you for the approval!