Skip to content

Commit a31d306

Browse files
mvzinkmvzink
committed
readyset-sql: Make variable/parameter conversion fallible
This was caught by logictests and some framework tests. Several framework tests hit one of these conditions, but I don't believe we'll inspect such statements anyway. If we do, we'll improve this conversion at a later date. Likely, improving this further will require fixing [sqlparser#1697]. [sqlparser#1697]: apache/datafusion-sqlparser-rs#1697 Change-Id: I2ddfc73a8b6d86d4aec8b4d4fefab86511a01b48
1 parent c83a961 commit a31d306

File tree

5 files changed

+93
-51
lines changed

5 files changed

+93
-51
lines changed

‎readyset-sql/src/ast/alter.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ impl TryFrom<sqlparser::ast::AlterColumnOperation> for AlterColumnOperation {
2323
match value {
2424
sqlparser::ast::AlterColumnOperation::SetDefault {
2525
value: sqlparser::ast::Expr::Value(value),
26-
} => Ok(Self::SetColumnDefault(value.into())),
26+
} => Ok(Self::SetColumnDefault(value.try_into()?)),
2727
sqlparser::ast::AlterColumnOperation::DropDefault => Ok(Self::DropColumnDefault),
2828
_ => unsupported!("ALTER COLUMN operation {value}"),
2929
}

‎readyset-sql/src/ast/expression.rs

+14-3
Original file line numberDiff line numberDiff line change
@@ -840,8 +840,19 @@ impl TryFromDialect<sqlparser::ast::Expr> for Expr {
840840
expr: _,
841841
collation: _,
842842
} => not_yet_implemented!("COLLATE"),
843-
// TODO: could this be a variable like `@@GLOBAL.foo`, which should go through `ident_into_expr` or similar?
844-
CompoundIdentifier(idents) => Ok(Self::Column(idents.into_dialect(dialect))),
843+
CompoundIdentifier(idents) => {
844+
let is_variable = if let Some(first) = idents.first() {
845+
first.quote_style.is_none() && first.value.starts_with('@')
846+
} else {
847+
false
848+
};
849+
if is_variable {
850+
Ok(Self::Variable(idents.try_into()?))
851+
} else {
852+
let column: Column = idents.into_dialect(dialect);
853+
Ok(Self::Column(column))
854+
}
855+
}
845856
Convert {
846857
expr: _,
847858
data_type: _,
@@ -1035,7 +1046,7 @@ impl TryFromDialect<sqlparser::ast::Expr> for Expr {
10351046
op: op.into(),
10361047
rhs: expr.try_into_dialect(dialect)?,
10371048
}),
1038-
Value(value) => Ok(Self::Literal(value.into())),
1049+
Value(value) => Ok(Self::Literal(value.try_into()?)),
10391050
CompoundFieldAccess {
10401051
root: _,
10411052
access_chain: _,

‎readyset-sql/src/ast/insert.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ impl TryFromDialect<sqlparser::ast::Insert> for InsertStatement {
4444
sqlparser::ast::SetExpr::Values(values) => {
4545
values.rows.try_into_dialect(dialect)?
4646
}
47-
_ => unimplemented!(), // Our AST currently doesn't support anything else
47+
body => return unsupported!("Unsupported source type for INSERT: {body}"),
4848
}
4949
} else {
5050
Vec::new()

‎readyset-sql/src/ast/literal.rs

+18-15
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use rust_decimal::Decimal;
1616
use serde::{Deserialize, Serialize};
1717
use test_strategy::Arbitrary;
1818

19-
use crate::{ast::*, Dialect, DialectDisplay};
19+
use crate::{ast::*, AstConversionError, Dialect, DialectDisplay};
2020

2121
#[derive(Clone, Debug, Serialize, Deserialize, Arbitrary)]
2222
pub struct Float {
@@ -216,42 +216,45 @@ impl From<ItemPlaceholder> for Literal {
216216
}
217217
}
218218

219-
impl From<sqlparser::ast::Value> for Literal {
220-
fn from(value: sqlparser::ast::Value) -> Self {
219+
impl TryFrom<sqlparser::ast::Value> for Literal {
220+
type Error = AstConversionError;
221+
222+
fn try_from(value: sqlparser::ast::Value) -> Result<Self, Self::Error> {
221223
use sqlparser::ast::Value;
222224
match value {
223-
Value::Placeholder(name) => Self::Placeholder(name.into()),
224-
Value::Boolean(b) => Self::Boolean(b),
225-
Value::Null => Self::Null,
225+
Value::Placeholder(name) => Ok(Self::Placeholder(name.into())),
226+
Value::Boolean(b) => Ok(Self::Boolean(b)),
227+
Value::Null => Ok(Self::Null),
226228
Value::DoubleQuotedString(s)
227229
| Value::SingleQuotedString(s)
228230
| Value::DoubleQuotedByteStringLiteral(s)
229-
| Value::SingleQuotedByteStringLiteral(s) => Self::String(s),
231+
| Value::SingleQuotedByteStringLiteral(s) => Ok(Self::String(s)),
230232
Value::DollarQuotedString(sqlparser::ast::DollarQuotedString { value, .. }) => {
231-
Self::String(value)
233+
Ok(Self::String(value))
232234
}
233235
Value::Number(s, _unknown) => {
234236
// TODO(mvzink): Probably should parse as unsigned first and/or fix nom-sql's
235237
// parsing of numeric literals. sqlparser-rs leaves the number as a string, and its
236238
// expression parsing will parse `-1` as `UnaryOp::Minus(Value::Number("1"))`.
237239
// However, to match nom-sql, we usually want a signed integer.
238240
if let Ok(i) = s.parse::<i64>() {
239-
Self::Integer(i)
241+
Ok(Self::Integer(i))
240242
} else if let Ok(i) = s.parse::<u64>() {
241-
Self::UnsignedInteger(i)
243+
Ok(Self::UnsignedInteger(i))
242244
} else if let Ok(f) = s.parse::<f64>() {
243-
Self::Double(crate::ast::Double {
245+
Ok(Self::Double(crate::ast::Double {
244246
value: f,
245247
precision: s.find('.').map(|i| (s.len() - i - 1) as u8).unwrap_or(0),
246-
})
248+
}))
247249
} else if let Ok(d) = Decimal::from_str_exact(&s) {
248250
// Seems like this will later get re-parsed the same way
249-
Self::Numeric(d.mantissa(), d.scale())
251+
Ok(Self::Numeric(d.mantissa(), d.scale()))
250252
} else {
251-
panic!("failed to parse number: {s}") // TODO: remember that 99% of this should be converted to TryFrom
253+
failed!("failed to parse number: {s}")
252254
}
253255
}
254-
_ => unimplemented!("unsupported literal {value:?}"),
256+
Value::EscapedStringLiteral(s) => Ok(Self::String(s)),
257+
_ => failed!("unsupported literal {value:?}"),
255258
}
256259
}
257260
}

‎readyset-sql/src/ast/set.rs

+59-31
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize};
66
use test_strategy::Arbitrary;
77

88
use crate::{
9-
ast::*, AstConversionError, Dialect, DialectDisplay, FromDialect, IntoDialect, TryFromDialect,
9+
ast::*, AstConversionError, Dialect, DialectDisplay, IntoDialect, TryFromDialect,
1010
TryIntoDialect,
1111
};
1212

@@ -36,10 +36,12 @@ impl TryFromDialect<sqlparser::ast::Statement> for SetStatement {
3636
let name = variables
3737
.into_iter()
3838
.exactly_one()
39-
.map(|mut object_name| object_name.0.pop().unwrap())
40-
.expect("Snowflake-style multiple variables not supported")
39+
.map_err(|_| failed_err!("Missing variable name"))?
40+
.0
41+
.pop()
42+
.unwrap()
4143
.into_dialect(dialect);
42-
let value: SetPostgresParameterValue = value.into_dialect(dialect);
44+
let value: SetPostgresParameterValue = value.try_into_dialect(dialect)?;
4345
let scope = if local {
4446
Some(PostgresParameterScope::Local)
4547
} else {
@@ -52,17 +54,18 @@ impl TryFromDialect<sqlparser::ast::Statement> for SetStatement {
5254
}))
5355
}
5456
Dialect::MySQL => {
55-
let name = variables
56-
.into_iter()
57-
.exactly_one()
58-
.expect("Snowflake-style multiple variables not supported");
57+
let name = variables.into_iter().exactly_one().map_err(|_| {
58+
unsupported_err!("Only single variable assignment supported")
59+
})?;
5960
Ok(Self::Variable(SetVariables {
6061
variables: vec![(
6162
name.try_into()?,
6263
value
6364
.into_iter()
6465
.exactly_one()
65-
.expect("Multiple variable assignments not supported")
66+
.map_err(|_| {
67+
unsupported_err!("Only single variable assignment supported")
68+
})?
6669
.try_into_dialect(dialect)?,
6770
)],
6871
}))
@@ -123,23 +126,28 @@ pub enum SetPostgresParameterValue {
123126
Value(PostgresParameterValue),
124127
}
125128

126-
impl FromDialect<Vec<sqlparser::ast::Expr>> for SetPostgresParameterValue {
127-
fn from_dialect(value: Vec<sqlparser::ast::Expr>, dialect: Dialect) -> Self {
129+
impl TryFromDialect<Vec<sqlparser::ast::Expr>> for SetPostgresParameterValue {
130+
fn try_from_dialect(
131+
value: Vec<sqlparser::ast::Expr>,
132+
dialect: Dialect,
133+
) -> Result<Self, AstConversionError> {
128134
if value.len() == 1 {
129135
if let sqlparser::ast::Expr::Identifier(sqlparser::ast::Ident { value, .. }) = &value[0]
130136
{
131137
if value.eq_ignore_ascii_case("DEFAULT") {
132-
return Self::Default;
138+
return Ok(Self::Default);
133139
}
134140
}
135141
}
136-
let values = value.into_iter().map(|expr| expr.into_dialect(dialect));
142+
let values = value.into_iter().map(|expr| expr.try_into_dialect(dialect));
137143
if values.len() == 1 {
138-
Self::Value(PostgresParameterValue::Single(
139-
values.exactly_one().unwrap(),
140-
))
144+
Ok(Self::Value(PostgresParameterValue::Single(
145+
values.exactly_one().unwrap()?,
146+
)))
141147
} else {
142-
Self::Value(PostgresParameterValue::List(values.collect()))
148+
Ok(Self::Value(PostgresParameterValue::List(
149+
values.try_collect()?,
150+
)))
143151
}
144152
}
145153
}
@@ -160,14 +168,17 @@ pub enum PostgresParameterValueInner {
160168
Literal(Literal),
161169
}
162170

163-
impl FromDialect<sqlparser::ast::Expr> for PostgresParameterValueInner {
164-
fn from_dialect(value: sqlparser::ast::Expr, dialect: Dialect) -> Self {
171+
impl TryFromDialect<sqlparser::ast::Expr> for PostgresParameterValueInner {
172+
fn try_from_dialect(
173+
value: sqlparser::ast::Expr,
174+
dialect: Dialect,
175+
) -> Result<Self, AstConversionError> {
165176
match value {
166-
sqlparser::ast::Expr::Value(value) => Self::Literal(value.into()),
177+
sqlparser::ast::Expr::Value(value) => Ok(Self::Literal(value.try_into()?)),
167178
sqlparser::ast::Expr::Identifier(ident) => {
168-
Self::Identifier(ident.into_dialect(dialect))
179+
Ok(Self::Identifier(ident.into_dialect(dialect)))
169180
}
170-
_ => unimplemented!("unsupported postgres parameter value {value:?}"),
181+
_ => unsupported!("unsupported Postgres parameter value {value:?}"),
171182
}
172183
}
173184
}
@@ -332,18 +343,20 @@ impl From<sqlparser::ast::Ident> for Variable {
332343
}
333344
}
334345

335-
impl TryFrom<sqlparser::ast::ObjectName> for Variable {
346+
impl TryFrom<Vec<sqlparser::ast::Ident>> for Variable {
336347
type Error = AstConversionError;
337348

338-
fn try_from(mut value: sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {
339-
let name = match value.0.pop().unwrap() {
340-
// XXX(mvzink): We lowercase across the board (even ignoring dialect) just to match nom-sql
341-
sqlparser::ast::ObjectNamePart::Identifier(ident) => ident.value.to_lowercase(),
342-
};
343-
if value.0.is_empty() {
349+
fn try_from(mut value: Vec<sqlparser::ast::Ident>) -> Result<Self, Self::Error> {
350+
// XXX(mvzink): We lowercase across the board (even ignoring dialect) just to match nom-sql
351+
let name = value
352+
.pop()
353+
.ok_or_else(|| failed_err!("Empty variable name"))?
354+
.value
355+
.to_lowercase();
356+
if value.is_empty() {
344357
Ok(name.into())
345-
} else if value.0.len() == 1 {
346-
let scope = value.0.pop().unwrap().into();
358+
} else if value.len() == 1 {
359+
let scope = value.pop().unwrap().value.as_str().into();
347360
Ok(Self {
348361
scope,
349362
name: name.into(),
@@ -354,6 +367,21 @@ impl TryFrom<sqlparser::ast::ObjectName> for Variable {
354367
}
355368
}
356369

370+
impl TryFrom<sqlparser::ast::ObjectName> for Variable {
371+
type Error = AstConversionError;
372+
373+
fn try_from(value: sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {
374+
value
375+
.0
376+
.into_iter()
377+
.map(|part| match part {
378+
sqlparser::ast::ObjectNamePart::Identifier(ident) => ident,
379+
})
380+
.collect::<Vec<_>>()
381+
.try_into()
382+
}
383+
}
384+
357385
#[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize, Arbitrary)]
358386
pub struct SetVariables {
359387
/// A list of variables and their assigned values

0 commit comments

Comments
 (0)