-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Replace HiveQlTranslation with a non-regex-based parser #4258
Conversation
cc02495
to
9edd087
Compare
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
8b094e4
to
54faac1
Compare
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Show resolved
Hide resolved
54faac1
to
71762ad
Compare
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveErrorCode.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveErrorCode.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
if (!input.hasNext()) { | ||
break; // skip to end-of-input error | ||
} | ||
// Don't handle escape sequences, just drop the backslash. |
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 seems wrong. We should fail if there are escape sequences if we're not going to handle them. Otherwise, we are returning a known invalid result.
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.
That's a good idea. I originally tried to figure out what escape sequences Hive supports, but eventually decided against it and ended up leaving the behavior the same as it was before.
I could pretty easily cover the common cases if we want, or I suspect Hive uses ...hive.ql.parse.BaseSemanticAnalyzer.unescapeSQLString
or something similar so I could try using that (I'd basically just search for the end of the string, pass the whole thing to that function, then escape only single quotes from that output).
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.
The first new commit rejects all escape sequences, and the second uses the function I mentioned above to translate all of them. I can squash them or drop the last one depending on what we want to do.
71762ad
to
74f6399
Compare
4d1aa49
to
33841f2
Compare
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.
Please squash the commits
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveQlToPrestoTranslator.java
Outdated
Show resolved
Hide resolved
8d6dd51
to
6f99d54
Compare
Squashed: - Use Hive's string unescaping to translate escape sequences in strings
6f99d54
to
4cb7387
Compare
CI failed -- #3161 |
Thanks! |
The new parser does the same thing the regular expression was supposed to do; it just replaces the quotation marks and makes sure that any contained quotation marks are properly escaped. No other escape sequences are handled (which will cause unexpected behavior if there's an escape sequence in a Hive view).
Also, the error thrown when parsing fails needs to be replaced, but I'm not sure what fits, besides adding a new error to
HiveErrorCode
. MaybeHIVE_PARSE_ERROR
?This addresses #3266.