-
Notifications
You must be signed in to change notification settings - Fork 998
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
PHOENIX-6365 Bogus AmbiguousTableException in query with aliases on l… #1133
base: master
Are you sure you want to change the base?
Conversation
@@ -893,15 +893,14 @@ public Void visit(NamedTableNode tableNode) throws SQLException { | |||
TableRef tableRef = createTableRef(connectionSchemaName, tableNode, true, false); | |||
PTable theTable = tableRef.getTable(); | |||
|
|||
String name = theTable.getName().getString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe extract this logic to a private helper function so it doesn't have to be duplicated in two places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This causes a few tests that try to access columns by un-aliased names to fail. https://crate.io/docs/sql-99/en/latest/chapters/18.html#correlation-name
I don't know if this behaviour has historical reasons, or a simple bug that got codified in the tests, but think that we should strive to sql compatible, and follow the standard. WDYT @gjacoby126 ? |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
wildcard selects with correlation names and local indexes still need to be fixed. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
I consider the patch finished now. Can you check again @gjacoby126 ? |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
…ocal indexed tables
Can you please check the finished patch again, @gjacoby126 ? |
💔 -1 overall
This message was automatically generated. |
Adding @kadirozde as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments and a couple of questions on local index handling part. Thanks @stoty, this is indeed nice work.
phoenix-core/src/main/java/org/apache/phoenix/compile/FromCompiler.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/compile/FromCompiler.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/compile/FromCompiler.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/compile/FromCompiler.java
Outdated
Show resolved
Hide resolved
@@ -226,6 +226,7 @@ private static void projectAllIndexColumns(StatementContext context, TableRef ta | |||
String indexColName = IndexUtil.getIndexColumnName(tableColumn); | |||
PColumn indexColumn = null; | |||
ColumnRef ref = null; | |||
boolean localIndexResolved = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be false
? Otherwise, this logic in catch block will never be true:
if(!localIndexResolved) {
throw e;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to swallow the exception when we have just created a new LocalIndexDataColumnRef column, as the current resolver won't recogise it.
We only want to throw an exception if we didn't create a new LocalIndexDataColumnRef, and the resolver fails for some other reason.
(That may be a non-existing case, but I wanted to make sure that I don't change the existing behaviour.)
@@ -298,6 +306,7 @@ private static void projectIndexColumnFamily(StatementContext context, String cf | |||
PColumn indexColumn = null; | |||
ColumnRef ref = null; | |||
String indexColumnFamily = null; | |||
boolean localIndexResolved = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above?
@@ -701,8 +723,7 @@ public Void visit(ProjectedColumnExpression expression) { | |||
indexProjectedColumns.add(expression); | |||
PColumn col = expression.getColumn(); | |||
// hack'ish... For covered columns with local indexes we defer to the server. | |||
if (col instanceof ProjectedColumn && ((ProjectedColumn) col) | |||
.getSourceColumnRef() instanceof LocalIndexDataColumnRef) { | |||
if (context.getDataColumns().contains(col)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain this change? I am not sure about this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the old and the new code checks wheter the column will be added from the base table by the coprocessor.
The old resolver returns a ProjectedColumn to a LocalIndexDataColumnRef, but the new resolver simply returns the projected column, so the logic wouldn't fire.
By checking if the column is in context.getDataColumns(), we check if a LocalIndexDataColumnRef has been created for this column, even if that is unwrapped by the time reach here.
Most of the non-test changes outside FromCompiler are actually compensating for this change in the resolver behaviour.
I wonder if there is a better way to change the resolver, so that we can get by with less changes to the code outside FromComplier.
phoenix-core/src/main/java/org/apache/phoenix/schema/LocalIndexDataColumnRef.java
Show resolved
Hide resolved
Some more analysis on possible alternative ways to solve this problem: The pre-patch resolver will always (at least in every case in the tests) will (incorrectly) include the data table, and will return the ProjectedColumn object that was generated when projecting the table. With the current patch, the resolver will not consider the data table in some cases, and the Column object will be re-generated by ColumnRef, and will be a straight PColumnImpl instance. The problematic columns are the wildcard columns in tables that are rewiritten to their local index tables, and need to be joined from the Base Data table by the coprocessor. |
💔 -1 overall
This message was automatically generated. |
…ocal indexed tables