Skip to content
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

Couple fixes #3110

Merged
merged 7 commits into from
Sep 5, 2017
Merged

Couple fixes #3110

merged 7 commits into from
Sep 5, 2017

Conversation

bbeaudreault
Copy link
Contributor

@sougou as discussed, this removes the column count doesn't match value count error when operating on a table with an auto increment. I had to avoid an "index out of range" panic by modifying extractColumnValues. Let me know what you think.

I also included a fix for @giaquinti in the parser for SET CHARACTER SET 'utf8'. I can move to another PR if you'd like but the change was trivial enough that it seemed worth avoiding the overhead.

@bbeaudreault
Copy link
Contributor Author

I added a small commit to ignore lc_messages per #3111

@@ -583,6 +587,9 @@ func analyzeOnDupExpressions(ins *sqlparser.Insert, pkIndex *schema.Index) (pkVa
func extractColumnValues(rowList sqlparser.Values, colnum int) (sqltypes.PlanValue, bool) {
pv := sqltypes.PlanValue{Values: make([]sqltypes.PlanValue, len(rowList))}
for i := 0; i < len(rowList); i++ {
if len(rowList[i])-1 < colnum {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is needed? I think this code is unreachable because if no value was specified, the colnum would have been computed as -1, and there is a higher level check that skips calling this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to fix a index out of range panic that occurred in my test without it. Do you think a better fix is in order?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have the call stack? I still can't see how it happened.

@bbeaudreault
Copy link
Contributor Author

@sougou ok I implemented an approach as we discussed. Let me know what you think.

While I was in there I converted all the errors in dml.go to vterrors.

@bbeaudreault
Copy link
Contributor Author

@sougou FYI I'm going to be away for the next week, but if you have any comments I might be able to address them tomorrow morning.

Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sinc you're going to be away, if there is an issue with bitval, I'll need to add a fix for it before this is submitted. Let me know if it's ok.

return NewStrVal(value.Raw()), nil
default:
return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "default value %v of type %v does not map to a sqlparser type", value.String(), value.Type())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think that the Bit to BitVal conversion may be incorrect. So, I'll need to look at how a Bit Value gets built and how it gets converted back. But I don't have time right now. I'll verify this later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this:

mysql> create table a(id int, val bit(8) default b'101', primary key(id));
Query OK, 0 rows affected (0.02 sec)

mysql>
mysql> describe a;
+-------+---------+------+-----+---------+-------+
| Field | Type    | Null | Key | Default | Extra |
+-------+---------+------+-----+---------+-------+
| id    | int(11) | NO   | PRI | 0       |       |
| val   | bit(8)  | YES  |     | b'101'  |       |
+-------+---------+------+-----+---------+-------+
2 rows in set (0.00 sec)

We have two problems:

  1. The sqltypes.Value we build for bit fields is incorrect here: https://github.com/youtube/vitess/blob/1595283c1e48b4c80ead69245292722e0ad4efc0/go/vt/vttablet/tabletserver/schema/schema.go#L159.
  2. The SQLVal we build here should just be a string representation, likely escaped because the data will be binary. Basically, the value should be treated like a regular quoted string.

There is also a more concise and efficient way to do this without the elaborate switch.

I'll patch this PR and add a new commit to make the above fixes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran an audit for default values for all data types. Only the bit type has a representation that is different from what mysql would otherwise return as a value. The more exotic geometry and json types don't allow default values. So, I'm thinking of just special-casing this.

Also, we don't have code to convert something like b'101' to the binary representation. So, I'm thinking of making mysql do this for us with select b'101'.

@sougou
Copy link
Contributor

sougou commented Sep 3, 2017

I didn't want to lose history on the conversation. So, instead of creating a brand new consolidated PR of yours and mine, I've created a PR to add to your branch: HubSpot#7.

The other option is to submit your changes to YouTube, and then immediately submit my PR.

PS: Merging my PR into yours will cause CLA bot to complain, but we can ignore it.

bbeaudreault and others added 7 commits September 5, 2017 08:30
@bbeaudreault
Copy link
Contributor Author

Very clever :) pulled in your fix, let's see how travis looks.

@sougou
Copy link
Contributor

sougou commented Sep 5, 2017

LGTM

Approved with PullApprove

@sougou sougou merged commit c3523e8 into vitessio:master Sep 5, 2017
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.

3 participants