PHOENIX-6365 Bogus AmbiguousTableException in query with aliases on l… #1133
+299
−124
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gjacoby126
requested changes
Feb 5, 2021
| @@ -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.
nit: maybe extract this logic to a private helper function so it doesn't have to be duplicated in two places
|
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. |
virajjasani
reviewed
Sep 5, 2021
There was a problem hiding this comment.
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.
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.
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; | |||
| @@ -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.
Could you please explain this change? I am not sure about this one.
There was a problem hiding this comment.
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ocal indexed tables