Skip to content
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

chore(spans): Upgrade sqlparser #3057

Merged
merged 4 commits into from
Feb 6, 2024
Merged

chore(spans): Upgrade sqlparser #3057

merged 4 commits into from
Feb 6, 2024

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Feb 5, 2024

The sqlparser lib has been on a fork since #2846. We can now go back to the official version because apache/datafusion-sqlparser-rs#1065 has been merged & released.

ref: https://github.com/getsentry/team-ingest/issues/272

@@ -383,7 +383,7 @@ mod tests {
scrub_sql_test!(
strip_prefixes_mysql_generic,
r#"SELECT `table`.`foo`, count(*) from `table` WHERE sku = %s"#,
r#"SELECT foo, count(*) from table WHERE sku = %s"#
r#"SELECT foo, count(*) FROM table WHERE sku = %s"#
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now parsed successfully and no longer relies on the fallback scrubber.

r#"REPLACE INTO `foo` (`a`) VALUES ("abcd1234")"#,
"REPLACE INTO foo (a) VALUES (%s)"
"REPLACE INTO foo (..) VALUES (%s)"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now parsed successfully and no longer relies on the fallback scrubber.

Comment on lines 208 to 235
TableFactor::Function {
name, args, alias, ..
} => {
Self::simplify_compound_identifier(&mut name.0);
for arg in args {
if let FunctionArg::Named { name, .. } = arg {
Self::scrub_name(name);
}
}
Self::simplify_table_alias(alias);
}
TableFactor::JsonTable { columns, alias, .. } => {
for col in columns {
Self::scrub_name(&mut col.name);
Self::simplify_table_alias(alias);
}
}
TableFactor::Unpivot {
value,
name,
columns,
alias,
..
} => {
Self::scrub_name(value);
Self::scrub_name(name);
Self::simplify_compound_identifier(columns);
Self::simplify_table_alias(alias);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enum now has three new variants.

Comment on lines +388 to +389
for operation in operations {
match operation {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have to loop through multiple operations, hence the large diff.

@jjbayer jjbayer marked this pull request as ready for review February 6, 2024 07:39
@jjbayer jjbayer requested a review from a team as a code owner February 6, 2024 07:39
TableFactor::JsonTable { columns, alias, .. } => {
for col in columns {
Self::scrub_name(&mut col.name);
Self::simplify_table_alias(alias);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to simplify the alias for every column?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@jjbayer jjbayer merged commit 3f98455 into master Feb 6, 2024
20 checks passed
@jjbayer jjbayer deleted the chore/sqlparser-043 branch February 6, 2024 12:09
jjbayer added a commit that referenced this pull request Feb 6, 2024
jjbayer added a commit that referenced this pull request Feb 6, 2024
jjbayer added a commit that referenced this pull request Feb 6, 2024
The `sqlparser` lib has been on a fork since
#2846. We can now go back to the
official version because
apache/datafusion-sqlparser-rs#1065 has been merged &
released.
jjbayer added a commit that referenced this pull request Feb 8, 2024
This PR reapplies #3057, which was
[reverted](#3059) because of
panics.

As a workaround, catch unwind panics so we can collect panic cases and
report them upstream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants