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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b27e500
chore(spans): Upgrade sqlparser
jjbayer Feb 5, 2024
bcb2c02
doc
jjbayer Feb 5, 2024
ea87502
fix
jjbayer Feb 5, 2024
0de38dc
ref: simplify outside of loop
jjbayer Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
- Drop spans ending outside the valid timestamp range. ([#3013](https://github.com/getsentry/relay/pull/3013))
- Extract INP metrics from spans. ([#2969](https://github.com/getsentry/relay/pull/2969), [#3041](https://github.com/getsentry/relay/pull/3041))
- Add ability to rate limit metric buckets by namespace. ([#2941](https://github.com/getsentry/relay/pull/2941))
- Upgrade sqlparser to 0.43.1.([#3057](https://github.com/getsentry/relay/pull/3057))

## 24.1.1

Expand Down
12 changes: 7 additions & 5 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 1 addition & 4 deletions relay-event-normalization/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,7 @@ serde = { workspace = true }
serde_json = { workspace = true }
serde_urlencoded = "0.7.1"
smallvec = { workspace = true }
# Use a fork of sqlparser until https://github.com/sqlparser-rs/sqlparser-rs/pull/1065 gets merged:
sqlparser = { git = "https://github.com/getsentry/sqlparser-rs.git", rev = "0bb7ec6ce661ecfc468f647fd1003b7839d37fa6", features = [
"visitor",
] }
sqlparser = { version = "0.43.1", features = ["visitor"] }
thiserror = { workspace = true }
url = { workspace = true }
uuid = { workspace = true }
Expand Down
11 changes: 4 additions & 7 deletions relay-event-normalization/src/normalize/span/description/sql/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

);

scrub_sql_test_with_dialect!(
Expand Down Expand Up @@ -581,7 +581,7 @@ mod tests {
scrub_sql_test!(
values_multi,
"INSERT INTO a (b, c, d, e) VALuES (%s, %s, %s, %s), (%s, %s, %s, %s), (%s, %s, %s, %s) ON CONFLICT DO NOTHING",
"INSERT INTO a (..) VALUES (%s) ON CONFLICT DO NOTHING"
"INSERT INTO a (..) VALUES (%s) ON CONFLICT DO NOTHING"
);

scrub_sql_test!(
Expand Down Expand Up @@ -876,13 +876,10 @@ mod tests {
);

scrub_sql_test_with_dialect!(
dont_fallback_to_regex,
replace_into,
"mysql",
// sqlparser cannot parse REPLACE INTO. If we know that
// a query is MySQL, we should give up rather than try to scrub
// with regex
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.

);

scrub_sql_test!(
Expand Down
193 changes: 114 additions & 79 deletions relay-event-normalization/src/normalize/span/description/sql/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use itertools::Itertools;
use once_cell::sync::Lazy;
use regex::Regex;
use sqlparser::ast::{
AlterTableOperation, Assignment, BinaryOperator, CloseCursor, ColumnDef, Expr, Ident,
ObjectName, Query, Select, SelectItem, SetExpr, Statement, TableAlias, TableConstraint,
AlterTableOperation, Assignment, BinaryOperator, CloseCursor, ColumnDef, Expr, FunctionArg,
Ident, ObjectName, Query, Select, SelectItem, SetExpr, Statement, TableAlias, TableConstraint,
TableFactor, UnaryOperator, Value, VisitMut, VisitorMut,
};
use sqlparser::dialect::{Dialect, GenericDialect};
Expand Down Expand Up @@ -198,12 +198,41 @@ impl VisitorMut for NormalizeVisitor {
Self::simplify_table_alias(alias);
}
TableFactor::Pivot {
table_alias,
pivot_alias,
value_column,
alias,
..
} => {
Self::simplify_compound_identifier(value_column);
Self::simplify_table_alias(alias);
}
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 column in columns {
Self::scrub_name(&mut column.name);
}
Self::simplify_table_alias(alias);
}
TableFactor::Unpivot {
value,
name,
columns,
alias,
..
} => {
Self::simplify_table_alias(table_alias);
Self::simplify_table_alias(pivot_alias);
Self::scrub_name(value);
Self::scrub_name(name);
Self::simplify_compound_identifier(columns);
Self::simplify_table_alias(alias);
}
}
ControlFlow::Continue(())
Expand Down Expand Up @@ -311,8 +340,10 @@ impl VisitorMut for NormalizeVisitor {
columns, source, ..
} => {
*columns = vec![Self::ellipsis()];
if let SetExpr::Values(v) = &mut *source.body {
v.rows = vec![vec![Expr::Value(Self::placeholder())]]
if let Some(source) = source.as_mut() {
if let SetExpr::Values(v) = &mut *source.body {
v.rows = vec![vec![Expr::Value(Self::placeholder())]]
}
}
}
// Simple lists of col = value assignments are collapsed to `..`.
Expand Down Expand Up @@ -350,89 +381,93 @@ impl VisitorMut for NormalizeVisitor {
Statement::Close {
cursor: CloseCursor::Specific { name },
} => Self::erase_name(name),
Statement::AlterTable { name, operation } => {
Statement::AlterTable {
name, operations, ..
} => {
Self::simplify_compound_identifier(&mut name.0);
match operation {
AlterTableOperation::AddConstraint(c) => match c {
TableConstraint::Unique { name, columns, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
for operation in operations {
match operation {
Comment on lines +388 to +389
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.

AlterTableOperation::AddConstraint(c) => match c {
TableConstraint::Unique { name, columns, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
}
for column in columns {
Self::scrub_name(column);
TableConstraint::ForeignKey {
name,
columns,
referred_columns,
..
} => {
if let Some(name) = name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
for column in referred_columns {
Self::scrub_name(column);
}
}
}
TableConstraint::ForeignKey {
name,
columns,
referred_columns,
..
} => {
if let Some(name) = name {
Self::scrub_name(name);
TableConstraint::Check { name, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
}
for column in columns {
Self::scrub_name(column);
TableConstraint::Index { name, columns, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
}
for column in referred_columns {
Self::scrub_name(column);
TableConstraint::FulltextOrSpatial {
opt_index_name,
columns,
..
} => {
if let Some(name) = opt_index_name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
}
},
AlterTableOperation::AddColumn { column_def, .. } => {
let ColumnDef { name, .. } = column_def;
Self::scrub_name(name);
}
TableConstraint::Check { name, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
AlterTableOperation::DropConstraint { name, .. } => Self::scrub_name(name),
AlterTableOperation::DropColumn { column_name, .. } => {
Self::scrub_name(column_name)
}
TableConstraint::Index { name, columns, .. } => {
if let Some(name) = name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
AlterTableOperation::RenameColumn {
old_column_name,
new_column_name,
} => {
Self::scrub_name(old_column_name);
Self::scrub_name(new_column_name);
}
TableConstraint::FulltextOrSpatial {
opt_index_name,
columns,
..
AlterTableOperation::ChangeColumn {
old_name, new_name, ..
} => {
if let Some(name) = opt_index_name {
Self::scrub_name(name);
}
for column in columns {
Self::scrub_name(column);
}
Self::scrub_name(old_name);
Self::scrub_name(new_name);
}
},
AlterTableOperation::AddColumn { column_def, .. } => {
let ColumnDef { name, .. } = column_def;
Self::scrub_name(name);
}
AlterTableOperation::DropConstraint { name, .. } => Self::scrub_name(name),
AlterTableOperation::DropColumn { column_name, .. } => {
Self::scrub_name(column_name)
}
AlterTableOperation::RenameColumn {
old_column_name,
new_column_name,
} => {
Self::scrub_name(old_column_name);
Self::scrub_name(new_column_name);
}
AlterTableOperation::ChangeColumn {
old_name, new_name, ..
} => {
Self::scrub_name(old_name);
Self::scrub_name(new_name);
}
AlterTableOperation::RenameConstraint { old_name, new_name } => {
Self::scrub_name(old_name);
Self::scrub_name(new_name);
}
AlterTableOperation::AlterColumn { column_name, .. } => {
Self::scrub_name(column_name);
AlterTableOperation::RenameConstraint { old_name, new_name } => {
Self::scrub_name(old_name);
Self::scrub_name(new_name);
}
AlterTableOperation::AlterColumn { column_name, .. } => {
Self::scrub_name(column_name);
}
_ => {}
}
_ => {}
}
}

Expand Down
Loading