You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "Aarchy (via GitHub)" <gi...@apache.org> on 2023/08/30 10:36:53 UTC
[GitHub] [phoenix] Aarchy opened a new pull request, #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Aarchy opened a new pull request, #1665:
URL: https://github.com/apache/phoenix/pull/1665
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#issuecomment-1708144872
> 1. I've checked oracle 23c version, strangely, it seems that it has the same faulty behavior, so the `columnLabel` and the `columnName` are the same in `ResultSetMetaData`. On the other hand, jdbc docs states:
>
> > getColumnLabel: Gets the designated column's suggested title for use in printouts and displays. The suggested title is usually specified by the SQL AS clause. If a SQL AS is not specified, the value returned from getColumnLabel will be the same as the value returned by the getColumnName method.
>
> 2. We discussed offline the upper/lowercase escaping problem, and come to the conclusion to not include `"` in the returned String to have the same behavior as before.
> 3. Test failures were caused by using cfamily wildcards ( `SELECT cf.*`), where the previous implementation returned the column name for both `getColumnLabel` and `getColumnName` (without `cf.`). With this change, `getColumnLabel` returns just the column name as before, but `getColumnName` returns the column family as well (see `QueryCompilerTest.java`). @stoty What do you think, what would be the correct behavior in this case?
I think that the patch's behaviour is fine, and follows the standard as much as possible.
We couldn't find the reason for removing the CF from the Label, but as this is the previous behaviour, and there is a comment in the code that confirms that this was the intended behaviour, we have left it as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#issuecomment-1708188140
Reading the JDCB javadoc, it says that isCaseSensitive refers to the column, not to the column name.
This is extremely underspecified, but in my interpretation, this means that the current implementation is incorrect.
Since Phoenix distinguish between case-sensitive and case-insensitve strings, I think that we should simply return "true" here.
We could just easily return false, as for comparison Phoenix uses standard case insensitive collation rules.
On second thought, I think that it's better to handle the isCaseSensitive issue in a separate JIRA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#discussion_r1325413783
##########
phoenix-core/src/main/java/org/apache/phoenix/compile/ColumnProjector.java:
##########
@@ -36,11 +36,17 @@
*/
public interface ColumnProjector {
/**
- * Get the column name as it was referenced in the query
Review Comment:
PQS and Connectors do not refer this class.
##########
phoenix-core/src/main/java/org/apache/phoenix/compile/ColumnProjector.java:
##########
@@ -36,11 +36,17 @@
*/
public interface ColumnProjector {
/**
- * Get the column name as it was referenced in the query
Review Comment:
We don't serialize ColumnProjectors, so we're good wrt backwards compatibility as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty closed pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty closed pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
URL: https://github.com/apache/phoenix/pull/1665
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#discussion_r1317119277
##########
phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixResultSetMetadataTest.java:
##########
@@ -54,18 +54,20 @@ public void testNullTypeName() throws Exception {
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 " +
+ "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, " +
- "\"v1\" AS \"testalias2\", v2 FROM T");
+ "\"v1\" AS \"testalias2\", v2, \"v3\" FROM T");
assertEquals("PK1", rs.getMetaData().getColumnName(1));
assertEquals("TESTALIAS1", rs.getMetaData().getColumnLabel(1));
assertEquals("PK2", rs.getMetaData().getColumnName(2));
assertEquals("PK2", rs.getMetaData().getColumnLabel(2));
- assertEquals("\"v1\"", rs.getMetaData().getColumnName(3));
+ assertEquals("v1", rs.getMetaData().getColumnName(3));
Review Comment:
can you add checks for the isCaseSensitive flag values as well ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#issuecomment-1717579036
There are a lot of test failures, which look relevant.
Please also check the checkstyle report (ignore the warnings for the existing lines)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] Aarchy commented on a diff in pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "Aarchy (via GitHub)" <gi...@apache.org>.
Aarchy commented on code in PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#discussion_r1321150235
##########
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:
Yes, this is used the same way as before, just gave a different name for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] Aarchy commented on pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "Aarchy (via GitHub)" <gi...@apache.org>.
Aarchy commented on PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#issuecomment-1708135039
1. I've checked oracle 23c version, strangely, it seems that it has the same faulty behavior, so the `columnLabel` and the `columnName` are the same in `ResultSetMetaData`. On the other hand, jdbc docs states:
> getColumnLabel: Gets the designated column's suggested title for use in printouts and displays. The suggested title is usually specified by the SQL AS clause. If a SQL AS is not specified, the value returned from getColumnLabel will be the same as the value returned by the getColumnName method.
2. We discussed offline the upper/lowercase escaping problem, and come to the conclusion to not include `"` in the returned String to have the same behavior as before.
3. Test failures were caused by using cfamily wildcards ( `SELECT cf.*`), where the previous implementation returned the column name for both `getColumnLabel` and `getColumnName` (without `cf.`). With this change, `getColumnLabel` returns just the column name as before, but `getColumnName` returns the column family as well (see `QueryCompilerTest.java`). @stoty What do you think, what would be the correct behavior in this case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] Aarchy commented on a diff in pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "Aarchy (via GitHub)" <gi...@apache.org>.
Aarchy commented on code in PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#discussion_r1317258995
##########
phoenix-core/src/test/java/org/apache/phoenix/jdbc/PhoenixResultSetMetadataTest.java:
##########
@@ -54,18 +54,20 @@ public void testNullTypeName() throws Exception {
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 " +
+ "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, " +
- "\"v1\" AS \"testalias2\", v2 FROM T");
+ "\"v1\" AS \"testalias2\", v2, \"v3\" FROM T");
assertEquals("PK1", rs.getMetaData().getColumnName(1));
assertEquals("TESTALIAS1", rs.getMetaData().getColumnLabel(1));
assertEquals("PK2", rs.getMetaData().getColumnName(2));
assertEquals("PK2", rs.getMetaData().getColumnLabel(2));
- assertEquals("\"v1\"", rs.getMetaData().getColumnName(3));
+ assertEquals("v1", rs.getMetaData().getColumnName(3));
Review Comment:
Added some test cases for it.
As it is very not straightforward what should this function return, created a separate [ticket](https://issues.apache.org/jira/browse/PHOENIX-7030) to find a proper solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#discussion_r1325408095
##########
phoenix-core/src/main/java/org/apache/phoenix/compile/ColumnProjector.java:
##########
@@ -36,11 +36,17 @@
*/
public interface ColumnProjector {
/**
- * Get the column name as it was referenced in the query
Review Comment:
Changing the semantics for getName() makes sense.
My concerns are:
- Does connectors or PQS refer it , and if yes is that a pronlem?
- Does this have backwards compatibility implications ?
My gut feeling is that it's never used outside the client context, but I will check this.
##########
phoenix-core/src/main/java/org/apache/phoenix/compile/ExpressionProjector.java:
##########
@@ -24,29 +24,31 @@
import org.apache.phoenix.expression.Expression;
import org.apache.phoenix.schema.types.PDataType;
import org.apache.phoenix.schema.tuple.Tuple;
-
+import org.apache.phoenix.util.SchemaUtil;
Review Comment:
This is not used, please remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#issuecomment-1700569949
Please check the test failures.
We've discussed the upper/lowercase escaping offline.
Could you check what results Oracle returns in that case, and try to approximate its behaviour ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] Aarchy commented on pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "Aarchy (via GitHub)" <gi...@apache.org>.
Aarchy commented on PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#issuecomment-1708332350
Jira ticket for isCaseSensitive issue: https://issues.apache.org/jira/browse/PHOENIX-7030
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on a diff in pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "stoty (via GitHub)" <gi...@apache.org>.
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on pull request #1665: PHOENIX-6920: PQS / thin client doesn't distinguish between label and column_name
Posted by "gjacoby126 (via GitHub)" <gi...@apache.org>.
gjacoby126 commented on PR #1665:
URL: https://github.com/apache/phoenix/pull/1665#issuecomment-1708545132
Looks like there are some BackwardCompatibilityIT failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org