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, mysql: return correct column name and column label name #7600

Merged
merged 4 commits into from
Sep 8, 2018

Conversation

eurekaka
Copy link
Contributor

@eurekaka eurekaka commented Sep 4, 2018

What problem does this PR solve?

Make returned column name and column label compatible with MySQL protocol/JDBC specification. Check out this example using JDBC to connect to TiDB:

import java.sql.*;

class TiDBConnector {
	public static void main(String args[]){
		try{
				Class.forName("com.mysql.jdbc.Driver");
				//here test is database name, root is username, and password is ""
				Connection con=DriverManager.getConnection("jdbc:mysql://127.0.0.1:4000/test","root","");
				Statement stmt=con.createStatement();
				ResultSet rs=stmt.executeQuery("select a as fake from t");
				rs.next();
				ResultSetMetaData metadata = rs.getMetaData();
				String nameField = metadata.getColumnName(1);
				String aliasField = metadata.getColumnLabel(1);
				System.out.println(nameField);
				System.out.println(aliasField);
				con.close();
		}catch(Exception e) { System.out.println(e); }
	}
}

before this PR, both nameField and aliasField are fake; after this PR, nameField is a, and aliasField is fake

What is changed and how it works?

distinguish column label and column name

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note: return correct column name and column label name

@winoros
Copy link
Member

winoros commented Sep 4, 2018

For a little complicated one select b as c from (select a as b from t) t;
The column name should still be a and the column label is c?

@eurekaka
Copy link
Contributor Author

eurekaka commented Sep 4, 2018

For expression support regarding this problem, it is more complicated. For example, if we are using this query:

select avg(a) as alias from t;

currently, parser would recognize the patterns in the sql text, build corresponding ASTs, but would not store the map between AST and original text, except for few expressions such as SubSelect, Field, CreateViewStmt etc.

In this case, avg(a) is the column name we expect. For Field, the stored original text in this case is avg(a) as alias, while the text avg(a) for corresponding AggregateFuncExpr AST is not stored.

Two approaches to get this avg(a) text:

  • backward traverse original text of Field, i.e, avg(a) as alias and remove the tailer part; this is ugly IMHO, and we should consider all patterns of alias declaration here, just like what parser does for FieldAsNameOpt;
  • build more maps between AST and original text in parser, for example, if we store text avg(a) for AggregateFuncExpr by calling SetText() method, since this AggregateFuncExpr is stored in SelectField::Expr, we can get the name of this SelectField by calling Text() method of SelectField::Expr, i.e, avg(a); attached is a simple demo patch to make avg work using this approach; this approach is more graceful than the first one, but we have to store the original text for a lot of expressions besides AggregateFuncExpr, this would cause trivial changes in parser.y, and these stored text is only useful in building this original column name.

Both approaches are not perfect from my perspective, considered that JDBC has clarified in 4.0 specification that users should use column label to get data, instead of column name, I prefer not to extent this PR to expressions now.

patch.tar.gz

@eurekaka
Copy link
Contributor Author

eurekaka commented Sep 4, 2018

@winoros MySQL and TiDB has same behavior after this PR: b as column name, c as column label.

@eurekaka
Copy link
Contributor Author

eurekaka commented Sep 4, 2018

/run-all-tests

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 5, 2018
@zz-jason
Copy link
Member

zz-jason commented Sep 5, 2018

@winoros @XuHuaiyu PTAL

@eurekaka
Copy link
Contributor Author

eurekaka commented Sep 7, 2018

@coocood PTAL

@coocood
Copy link
Member

coocood commented Sep 8, 2018

LGTM

@coocood coocood added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 8, 2018
@coocood coocood merged commit b3d4ed7 into pingcap:master Sep 8, 2018
@eurekaka eurekaka deleted the origin_col_name branch September 25, 2018 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/mysql-protocol status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants