-
Notifications
You must be signed in to change notification settings - Fork 74
Ensure we output transpiled code #2014
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
base: main
Are you sure you want to change the base?
Conversation
✅ 27/27 passed, 1 flaky, 1m20s total Flaky tests:
Running from acceptance #2340 |
I don't think this is the proper way to go here. When there is some parsing error, morpheus doesn't currently guarantee it won't "cut" some of its input. For instance, when attempting to transpile ADD SIGNATURE TO ProcForAlice BY CERTIFICATE csSelectT which uses syntax that isn't currently covered by the grammar, morpheus will output the following /* Parse error: unexpected extra input 'ADD' while parsing a sqlFile
expecting one of: @Local, End of batch, Identifier, Node ID, Select Statement, Statement, '(', ';', 'ARRAY', 'BULK', 'CALL', 'DBCC'...
'TO' was unexpected while parsing a sqlFile
expecting one of: End of batch, Select Statement, Statement, '(', 'BULK', 'CALL', 'COMMENT', 'CONTRACT', 'DBCC', 'DISABLE', 'ENABLE', 'GET'... */
-- FIXME: Unparsed input - ErrorNode encountered
;
/* SIGNATURE */
-- FIXME: SNOWFLAKE: The transpiler cannnot currently convert Execute body batch, but may be able to do so in the future
; where most of the input query has disappeared (in this instance, we've lost everything but the What I suggest we do instead is to keep outputting |
@vil1 I do like your idea more as the long-term solution, although we need to think about how we would handle the output from the BB converter. I still think this initial change is a step in the right direction. Outputting the input source with 0 context is very disorienting. |
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.
LGTM, may be an additional tests for this behavoiur will show an example of how morpheus behaves for failures.
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.
Ready to approve once my little comment is addressed and https://github.com/databrickslabs/morpheus/pull/509 is merged
|
||
if any(err.kind == ErrorKind.PARSING for err in error_list): | ||
output_code = context.source_code or "" | ||
output_code = context.transpiled_code or "" |
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 should remove the whole if
What does this PR do?
Currently, if there's a parsing error (usually, these mean the output is unusable) we copy the input code into the output file. This leads to a confusing interaction, because there is no indication that the file is the same as the input, and the user is left wondering what happened.
This PR changes the output, so that the transpiled code, including all issues we found, are available in the "transpiled" file.
Tests