-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: split_with_parser, split_with_scanner #6
feat: split_with_parser, split_with_scanner #6
Conversation
I'm noticing some weirdness in the split_with_scanner example. Malformed tokens seem to be getting dropped.
Yup, that's expected, if the statement only contains invalid tokens, we skip it here: https://github.com/pganalyze/libpg_query/blob/13-latest/src/pg_query_split.c#L113 We could change that behavior, its not something that anyone relies on today, to my knowledge. Would it be helpful for your use case? |
Cool, I'll update my comment on the function. |
Great, and neat idea! I'll leave the Rust code review to @seanlinsley, since he's been maintaining most of the library, but looks good from a quick glance :) |
#[error("Error scanning: {0}")] | ||
Scan(String), | ||
#[error("Error splitting: {0}")] | ||
Split(String), |
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.
@lfittl are there cases where these functions can raise unique error messages, or should we be using the existing Parse
error type instead?
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.
Good question. There are no unique Split errors (at least not in the C library), so we could rely on Parse
error for split_with_parser
.
For split_with_scanner
it seems reasonable to have its own error class, since we don't actually call the parser there, so it'd be incorrect to treat errors as "parse errors".
For reference, here is the rather simple code that does the splitting in C: https://github.com/pganalyze/libpg_query/blob/13-latest/src/pg_query_split.c
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.
👍 in general, just not sure if we need the extra error types
@seanlinsley would you be willing to approve workflows on this PR so we're all looking at the same unit test output? |
Thanks for submitting this PR @SKalt! |
I'm noticing some weirdness in the split_with_scanner example. Malformed tokens seem to be getting dropped:
Is this expected?
Resolves #5.