server: fix type year has too many 0s in prepare/execute

[imtbkcat]

What problem does this PR solve?

Fix type year has too many 0s in prepare/execute.

What is changed and how it works?

Add a condition to avoid multiply mysql.MaxBytesOfCharacter for number type.

Before adding condition, using test code below will receive result 0000000000001999, but expect 1999. This is because ci.ColumnLength * mysql.MaxBytesOfCharacter, the value should be 4 not 16.

Check List

Tests

• Unit test
• Manual test (add detailed scripts or steps below)

Test code:

import java.io.*;
import java.sql.*;
import java.util.*;
import com.mysql.jdbc.*;
import java.lang.reflect.Method; 
  

public class Main {
    String dbUrl = "jdbc:mysql://localhost:4000/test?user=root&password=&useSSL=false&useServerPrepStmts=true";

    Connection getConnectionWithProps(String propsList) throws SQLException {
        return getConnectionWithProps(dbUrl, propsList);
    }

    Connection getConnectionWithProps(String url, String propsList) throws SQLException {
        Properties props = new Properties();
         if (propsList != null) {
        List<String> keyValuePairs = StringUtils.split(propsList, ",", false);
         for (String kvp : keyValuePairs) {
            List<String> splitUp = StringUtils.split(kvp, "=", false);
            StringBuilder value = new StringBuilder();
             for (int i = 1; i < splitUp.size(); i++) {
                if (i != 1) {
                    value.append("=");
                }
                 value.append(splitUp.get(i));
             }
             props.setProperty(splitUp.get(0).toString().trim(), value.toString());
        }
    }
     return getConnectionWithProps(url, props);
}

protected Connection getConnectionWithProps(String url, Properties props) throws SQLException {
    return DriverManager.getConnection(url, props);
}

protected Connection getConnectionWithProps(Properties props) throws SQLException {
    return DriverManager.getConnection(dbUrl, props);
}

void run() throws SQLException {
    Connection noSyncConn = null;
    Properties props = new Properties();
    props.setProperty("noDatetimeStringSync", "true");
    props.setProperty("useUsageAdvisor", "true");
    props.setProperty("yearIsDateType", "false");
    noSyncConn = getConnectionWithProps(props);
	ResultSet rs = null;
    //rs = noSyncConn.createStatement().executeQuery("SELECT field1, field2 FROM testBug8428");
    rs = noSyncConn.prepareStatement("SELECT field1 FROM testBug8428").executeQuery();
    rs.next();
    ResultSetMetaData rsmd=rs.getMetaData();
    System.out.println(rs.getString(1));
}

public static void main(String[] args) throws SQLException {
	new Main().run();
}
}

Expect:
1999

Code changes

• Has exported variable/fields change

Side effects

• Possible performance regression

[jackysp]

I think we can only use types.IsString(fld.Column.Tp).
Could we test the cases in the comments at line 395 to 399, maybe we need a Navicat.

[imtbkcat]

I will have a try.

[zhexuany]

In PR's description, you say this fixes too many leading 0 before year. I am confused why you check column type is string or not.

[imtbkcat]

Because too many leading zero may also appear when column type is a number type and has ZerofillFlag.

[jackysp]

@zhexuany , according to the comments below, this else branch is only used for the show create table results when it meets Navicat. I think adding a string type check is suitable here.
You could blame the history of this line, it caused at least 2 issues in the past.

[coocood]

@imtbkcat
The types.IsString covers the previous conditions, we can remove them.

[imtbkcat]

@coocood
Get it.

[zhexuany]

"according to the comments below, this else branch is only used for the show create table results when it meets Navicat." I had read this part and understand this change made to make navicat happy. While the problem stil remains open: why do we only check the column type is string or not? The reason I ask is to clarify this. And I think it is worth to be commented.

[imtbkcat]

/run-all-tests

[coocood]

LGTM

[jackysp]

LGTM

one more issue need to be addressed.

[zhexuany]

My original question still remains open. Why does types.IsString(fld.Column.Tp) work for year? I understood this is used for solving the compatibility issue we found in Navicat.
At least, it is worth a comment here.

[coocood]

I think this is already explained in the PR description, we don't need to explain why old code causes some bug, because old code is removed.

[imtbkcat]

Firstly, year is a number typer, so it will not multiply mysql.MaxBytesOfCharacter.
Secondly, using types.IsString(fld.Column.Tp) rather than just fld.Column.Tp != mysql.TypeYear can fix some other problem cause by this if branch.

[imtbkcat]

@zhexuany , For example,

create table t (k smallint(10) unsigned zerofill);
insert into t value (10);

using jdbc test code above, TiDB and MySQL has different result.
MySQL: 0000000010
TiDB: 0000000000000000000000000000000000000010

This mean number type meet zerofill will has this problem. So we should use types.IsString(fld.Column.Tp).

[zhexuany]

we don't need to explain why old code causes some bug, because old code is removed.

But we need why the fixes work?. Information still resides at PR not code itself.

When people read this code, they probably have no idea about this change. They have to go through PR which is a form of cost. I believe information has to be togehter with the code. That is why we need comment.

[imtbkcat]

I will add some comment.

[zhexuany]

Yes. We only need check string type simply because string type has truncation problem. This comment can provide context which can help other people better understand.