You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2023/09/08 13:06:00 UTC
[jira] [Commented] (PHOENIX-6920) PQS / thin client doesn't distinguish between label and column_name
[ https://issues.apache.org/jira/browse/PHOENIX-6920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17763084#comment-17763084 ]
ASF GitHub Bot commented on PHOENIX-6920:
-----------------------------------------
stoty commented on code in PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#discussion_r1319836291
##########
phoenix-core/src/test/java/org/apache/phoenix/compile/QueryMetaDataTest.java:
##########
@@ -309,7 +309,7 @@ public void testBasicResultSetMetaData() throws Exception {
assertEquals("organization_id".toUpperCase(),md.getColumnName(1));
assertEquals("a_string".toUpperCase(),md.getColumnName(2));
assertEquals("b_string".toUpperCase(),md.getColumnName(3));
- assertEquals("i".toUpperCase(),md.getColumnName(4));
+ assertEquals("a_integer".toUpperCase(),md.getColumnName(4));
Review Comment:
check the alias as well.
##########
phoenix-core/src/main/java/org/apache/phoenix/compile/ExpressionProjector.java:
##########
@@ -39,14 +39,24 @@ public class ExpressionProjector implements ColumnProjector {
private final Expression expression;
private final String tableName;
private final boolean isCaseSensitive;
-
+ private final String label;
+
+ public ExpressionProjector(String name, String tableName, Expression expression, boolean isCaseSensitive, String label) {
+ this.name = name;
+ this.expression = expression;
+ this.tableName = tableName;
+ this.isCaseSensitive = isCaseSensitive;
+ this.label = label;
+ }
+
public ExpressionProjector(String name, String tableName, Expression expression, boolean isCaseSensitive) {
Review Comment:
What is the resaon for keeping this ?
##########
phoenix-core/src/main/java/org/apache/phoenix/compile/ExpressionProjector.java:
##########
@@ -39,14 +39,24 @@ public class ExpressionProjector implements ColumnProjector {
private final Expression expression;
private final String tableName;
private final boolean isCaseSensitive;
-
+ private final String label;
+
+ public ExpressionProjector(String name, String tableName, Expression expression, boolean isCaseSensitive, String label) {
Review Comment:
nit: name and label are strongly related, I'd prefer if they were consecutive parameters.
##########
phoenix-core/src/main/java/org/apache/phoenix/compile/ExpressionProjector.java:
##########
@@ -39,14 +39,24 @@ public class ExpressionProjector implements ColumnProjector {
private final Expression expression;
private final String tableName;
private final boolean isCaseSensitive;
-
+ private final String label;
+
+ public ExpressionProjector(String name, String tableName, Expression expression, boolean isCaseSensitive, String label) {
+ this.name = name;
+ this.expression = expression;
+ this.tableName = tableName;
+ this.isCaseSensitive = isCaseSensitive;
+ this.label = label;
+ }
+
public ExpressionProjector(String name, String tableName, Expression expression, boolean isCaseSensitive) {
Review Comment:
Having a single construnctor which requires all parameters would make it harder to introduce new bugs.
##########
phoenix-core/src/main/java/org/apache/phoenix/compile/ProjectionCompiler.java:
##########
@@ -470,10 +470,24 @@ public static RowProjector compile(StatementContext context, SelectStatement sta
ExpressionCompiler.throwNonAggExpressionInAggException(expression.toString());
}
}
- String columnAlias = aliasedNode.getAlias() != null ? aliasedNode.getAlias() : SchemaUtil.normalizeIdentifier(aliasedNode.getNode().getAlias());
- boolean isCaseSensitive = aliasedNode.getAlias() != null ? aliasedNode.isCaseSensitve() : (columnAlias != null ? SchemaUtil.isCaseSensitive(aliasedNode.getNode().getAlias()) : selectVisitor.isCaseSensitive);
- String name = columnAlias == null ? expression.toString() : columnAlias;
- projectedColumns.add(new ExpressionProjector(name, tableRef.getTableAlias() == null ? (table.getName() == null ? "" : table.getName().getString()) : tableRef.getTableAlias(), expression, isCaseSensitive));
+
+ String tableName = tableRef.getTableAlias() == null ?
+ (table.getName() == null ?
+ "" :
+ table.getName().getString()) :
+ tableRef.getTableAlias();
+ String colName = SchemaUtil.normalizeIdentifier(aliasedNode.getNode().getAlias());
Review Comment:
Does this work because the the getAlias() here is not the alias set in select command, and is only set for direct columns where we omit the column family ?
##########
phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixResultSetMetadataTest.java:
##########
@@ -49,4 +51,80 @@ public void testNullTypeName() throws Exception {
assertEquals("NULL", rs.getMetaData().getColumnTypeName(1));
}
+
+ @Test
+ public void testCaseSensitiveExpression() throws Exception {
+ Connection conn = DriverManager.getConnection(getUrl());
+ conn.createStatement().execute(
+ "CREATE TABLE T (pk1 CHAR(15) not null, pk2 VARCHAR not null, \"v1\" VARCHAR(15), v2 DATE, \"v3\" VARCHAR " +
+ "CONSTRAINT pk PRIMARY KEY (pk1, pk2)) ");
+ ResultSet rs = conn.createStatement().executeQuery("SELECT pk1 AS testalias1, pk2 AS \"testalias2\", " +
+ "\"v1\" AS \"testalias3\", v2, \"v3\" FROM T");
+
+ assertEquals("PK1", rs.getMetaData().getColumnName(1));
+ assertEquals("TESTALIAS1", rs.getMetaData().getColumnLabel(1));
+ assertFalse(rs.getMetaData().isCaseSensitive(1));
+
+ assertEquals("PK2", rs.getMetaData().getColumnName(2));
+ assertEquals("testalias2", rs.getMetaData().getColumnLabel(2));
+ assertTrue(rs.getMetaData().isCaseSensitive(2));
+
+ assertEquals("v1", rs.getMetaData().getColumnName(3));
+ assertEquals("testalias3", rs.getMetaData().getColumnLabel(3));
+ assertTrue(rs.getMetaData().isCaseSensitive(3));
+
+ assertEquals("V2", rs.getMetaData().getColumnName(4));
+ assertEquals("V2", rs.getMetaData().getColumnLabel(4));
+ assertFalse(rs.getMetaData().isCaseSensitive(4));
+
+ assertEquals("v3", rs.getMetaData().getColumnName(5));
+ assertEquals("v3", rs.getMetaData().getColumnLabel(5));
+ assertTrue(rs.getMetaData().isCaseSensitive(5));
+ }
+
+ @Test
Review Comment:
Can you add a separate test for the VIEW case that was found by BackwardCompatibilityIT ?
> PQS / thin client doesn't distinguish between label and column_name
> -------------------------------------------------------------------
>
> Key: PHOENIX-6920
> URL: https://issues.apache.org/jira/browse/PHOENIX-6920
> Project: Phoenix
> Issue Type: Bug
> Components: queryserver
> Reporter: Istvan Toth
> Assignee: Aron Attila Meszaros
> Priority: Major
>
> While debugging PHOENIX-6917, I found that we receive the aliased column name in both the _column_name_ and _label_ fields of the column metadata in protobuf.
> This happened with PQS head and Phoenix 5.2.0-SNAPSHOT.
> Apparently, some older version used to work correctly (and returned the unaliased name in {_}column_name{_})
> We need to check if this is a Phoenix or Avatica regression first, then fix it in the appropriate project.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)