Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6365 Bogus AmbiguousTableException in query with aliases on l… - #1133
Open
stoty wants to merge 3 commits into
Open
PHOENIX-6365 Bogus AmbiguousTableException in query with aliases on l…#1133stoty wants to merge 3 commits into
stoty wants to merge 3 commits into
Conversation
gjacoby126
requested changes
Feb 5, 2021
| TableRef tableRef = createTableRef(connectionSchemaName, tableNode, true, false); | ||
| PTable theTable = tableRef.getTable(); | ||
| String name = theTable.getName().getString(); |
Contributor
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
stoty
commented
Feb 5, 2021
ContributorAuthor
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 ? |
stoty
commented
Feb 5, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 5, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 7, 2021
ContributorAuthor
wildcard selects with correlation names and local indexes still need to be fixed. |
stoty
commented
Feb 7, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 7, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 9, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 9, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 10, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 10, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 19, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Feb 22, 2021
ContributorAuthor
I consider the patch finished now. Can you check again @gjacoby126 ? |
stoty
marked this pull request as draft
July 15, 2021 05:31
stoty
commented
Aug 5, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Aug 5, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
…ocal indexed tables
stoty
commented
Aug 31, 2021
ContributorAuthor
Can you please check the finished patch again, @gjacoby126 ? |
stoty
marked this pull request as ready for review
August 31, 2021 14:29
stoty
commented
Aug 31, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
virajjasani
commented
Sep 5, 2021
Contributor
Adding @kadirozde as well. |
virajjasani
left a comment
Contributor
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| String indexColName = IndexUtil.getIndexColumnName(tableColumn); | ||
| PColumn indexColumn = null; | ||
| ColumnRef ref = null; | ||
| boolean localIndexResolved = true; |
Contributor
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;
}
ContributorAuthor
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.)
| PColumn indexColumn = null; | ||
| ColumnRef ref = null; | ||
| String indexColumnFamily = null; | ||
| boolean localIndexResolved = true; |
| // 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)) { |
Contributor
There was a problem hiding this comment.
Could you please explain this change? I am not sure about this one.
ContributorAuthor
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.
Uh oh!
There was an error while loading. Please reload this page.
ContributorAuthor
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. |
stoty
commented
Sep 7, 2021
ContributorAuthor
💔 -1 overall
This message was automatically generated. |
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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