skip presto view parsing #211
Conversation
waggle-dance-core/src/main/java/com/hotels/bdp/waggledance/mapping/model/ASTQueryMapping.java
Outdated
Show resolved
Hide resolved
| String databaseName = metaStoreMapping.transformOutboundDatabaseName(originalDatabaseName); | ||
| table.setDbName(databaseName); | ||
| if (databaseName.equalsIgnoreCase(originalDatabaseName)) { | ||
| // Skip all the view parsing if nothing is going to change, the parsing is not without problems and we can't catch |
There was a problem hiding this comment.
is it worth keeping the code on line 123 since that will throw an error still?
There was a problem hiding this comment.
the original can't have the same string as the extended cause it also needs the colon. if we add it will it still throw an error?
There was a problem hiding this comment.
I return the table in this if so we'll not even try to parse.
There was a problem hiding this comment.
yes, but if the database name is different it will still throw an error
There was a problem hiding this comment.
No because I skip parsing in the parser class for Presto markers. The if in here is to ensure other crazy markers don't also through errors. The code here is to basically get back to same support as in pre WD 3.6.0. WD 3.7.0 introduced the problems.
If we really have to support Presto I got this follow up: #212
There was a problem hiding this comment.
OK, thanks for creating that issue. So for now we'll return the Presto view "as is" but that could mean it won't actually work since it doesn't have the correct prefixes and that's what #212 is all about.
There was a problem hiding this comment.
correct this is fixing and reverting old behaviour. Make it work again if there are no prefixes to remove.
...e-dance-core/src/test/java/com/hotels/bdp/waggledance/mapping/model/ASTQueryMappingTest.java
Outdated
Show resolved
Hide resolved
...e-dance-core/src/test/java/com/hotels/bdp/waggledance/mapping/model/ASTQueryMappingTest.java
Outdated
Show resolved
Hide resolved
| String originalDatabaseName = table.getDbName(); | ||
| String databaseName = metaStoreMapping.transformOutboundDatabaseName(originalDatabaseName); | ||
| table.setDbName(databaseName); | ||
| if (databaseName.equalsIgnoreCase(originalDatabaseName)) { |
There was a problem hiding this comment.
I don't understand what's going on here - why is the db name the same in this case?
There was a problem hiding this comment.
I don't know if it is that's true that's why I'm checking:
if our transform didn't change the database name we don't need to try and do the parsing just return the table as is (nothing would have changed).
This creeped in when we removed the IdentityMapping (which was just returning table for non prefixed metastores). So the if will mimic that behaviour again.
…ping/model/ASTQueryMapping.java Co-authored-by: Andreea Paduraru <40387422+andreeapad@users.noreply.github.com>
Co-authored-by: Andreea Paduraru <40387422+andreeapad@users.noreply.github.com>
…ping/model/ASTQueryMappingTest.java Co-authored-by: Adrian Woodhead <awoodhead@expediagroup.com>
…ping/model/ASTQueryMappingTest.java Co-authored-by: Adrian Woodhead <awoodhead@expediagroup.com>
| @@ -56,6 +61,13 @@ public String transformOutboundDatabaseName(MetaStoreMapping metaStoreMapping, S | |||
| return result.toString(); | |||
| } | |||
|
|
|||
| boolean hasMarker(String query) { | |||
There was a problem hiding this comment.
The function name hasMarker isn't super descriptive. Can we change it to something like hasIncompatibleViewMarker, or hasPrestoViewMarker, or hasNonHiveViewMarker, or something like that?
There was a problem hiding this comment.
sure I like hasNonHiveViewMarker
No description provided.