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

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

Merged
merged 3 commits into from
Aug 30, 2018
Merged

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

merged 3 commits into from
Aug 30, 2018

Conversation

imtbkcat
Copy link

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

@@ -391,7 +391,7 @@ func convertColumnInfo(fld *ast.ResultField) (ci *ColumnInfo) {
// Consider the decimal point.
ci.ColumnLength++
}
} else if fld.Column.Tp != mysql.TypeBit && fld.Column.Tp != mysql.TypeTiny {
} else if fld.Column.Tp != mysql.TypeBit && fld.Column.Tp != mysql.TypeTiny && types.IsString(fld.Column.Tp) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I will have a try.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

@coocood
Get it.

Copy link
Contributor

Choose a reason for hiding this comment

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

"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
Copy link
Author

/run-all-tests

@coocood
Copy link
Member

coocood commented Aug 29, 2018

LGTM

jackysp
jackysp previously approved these changes Aug 30, 2018
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@zhexuany zhexuany dismissed jackysp’s stale review August 30, 2018 03:08

one more issue need to be addressed.

@@ -391,7 +391,7 @@ func convertColumnInfo(fld *ast.ResultField) (ci *ColumnInfo) {
// Consider the decimal point.
ci.ColumnLength++
}
} else if fld.Column.Tp != mysql.TypeBit && fld.Column.Tp != mysql.TypeTiny {
} else if types.IsString(fld.Column.Tp) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

@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).

Copy link
Contributor

@zhexuany zhexuany Aug 30, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I will add some comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@coocood coocood merged commit 4a31302 into pingcap:master Aug 30, 2018
@imtbkcat imtbkcat deleted the toomany0 branch August 30, 2018 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1. type/compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants