Skip to content

Commit

Permalink
fix: Don't silently accept multi-table FROM clauses (eg: implicit JOI…
Browse files Browse the repository at this point in the history
…N syntax)
  • Loading branch information
alexander-beedie committed Jun 17, 2024
1 parent be5c3de commit 4bd0442
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 18 deletions.
18 changes: 11 additions & 7 deletions crates/polars-sql/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ impl SQLContext {
}

pub(crate) fn execute_query_no_ctes(&mut self, query: &Query) -> PolarsResult<LazyFrame> {
let lf = self.process_set_expr(&query.body, query)?;
let lf = self.process_query(&query.body, query)?;
self.process_limit_offset(lf, &query.limit, &query.offset)
}

Expand Down Expand Up @@ -263,7 +263,7 @@ impl SQLContext {
}
}

fn process_set_expr(&mut self, expr: &SetExpr, query: &Query) -> PolarsResult<LazyFrame> {
fn process_query(&mut self, expr: &SetExpr, query: &Query) -> PolarsResult<LazyFrame> {
match expr {
SetExpr::Select(select_stmt) => self.execute_select(select_stmt, query),
SetExpr::Query(query) => self.execute_query_no_ctes(query),
Expand Down Expand Up @@ -326,8 +326,8 @@ impl SQLContext {
} => (JoinType::Anti, "EXCEPT"),
_ => (JoinType::Semi, "INTERSECT"),
};
let mut lf = self.process_set_expr(left, query)?;
let mut rf = self.process_set_expr(right, query)?;
let mut lf = self.process_query(left, query)?;
let mut rf = self.process_query(right, query)?;
let join = lf
.clone()
.join_builder()
Expand Down Expand Up @@ -364,8 +364,8 @@ impl SQLContext {
quantifier: &SetQuantifier,
query: &Query,
) -> PolarsResult<LazyFrame> {
let mut lf = self.process_set_expr(left, query)?;
let mut rf = self.process_set_expr(right, query)?;
let mut lf = self.process_query(left, query)?;
let mut rf = self.process_query(right, query)?;
let opts = UnionArgs {
parallel: true,
to_supertypes: true,
Expand Down Expand Up @@ -586,7 +586,11 @@ impl SQLContext {
let mut lf = if select_stmt.from.is_empty() {
DataFrame::empty().lazy()
} else {
self.execute_from_statement(select_stmt.from.first().unwrap())?
let from = select_stmt.clone().from;
if from.len() > 1 {
polars_bail!(SQLInterface: "multiple tables in FROM clause are not currently supported (found {}); use explicit JOIN syntax instead", from.len())
}
self.execute_from_statement(from.first().unwrap())?
};
let mut contains_wildcard = false;
let mut contains_wildcard_exclude = false;
Expand Down
22 changes: 11 additions & 11 deletions crates/polars-sql/src/sql_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ impl SQLExprVisitor<'_> {
},
},
other => {
polars_bail!(SQLInterface: "SQL operator {:?} is not currently supported", other)
polars_bail!(SQLInterface: "operator {:?} is not currently supported", other)
},
})
}
Expand Down Expand Up @@ -772,7 +772,7 @@ impl SQLExprVisitor<'_> {
bitstring_to_bytes_literal(b)?
},
SQLValue::SingleQuotedString(s) => lit(s.clone()),
other => polars_bail!(SQLInterface: "SQL value {:?} is not supported", other),
other => polars_bail!(SQLInterface: "value {:?} is not supported", other),
})
}

Expand Down Expand Up @@ -826,7 +826,7 @@ impl SQLExprVisitor<'_> {
SQLValue::SingleQuotedString(s) | SQLValue::DoubleQuotedString(s) => {
AnyValue::StringOwned(s.into())
},
other => polars_bail!(SQLInterface: "SQL value {:?} is not currently supported", other),
other => polars_bail!(SQLInterface: "value {:?} is not currently supported", other),
})
}

Expand All @@ -845,11 +845,11 @@ impl SQLExprVisitor<'_> {

let low = self.convert_temporal_strings(&expr, &low);
let high = self.convert_temporal_strings(&expr, &high);
if negated {
Ok(expr.clone().lt(low).or(expr.gt(high)))
Ok(if negated {
expr.clone().lt(low).or(expr.gt(high))
} else {
Ok(expr.clone().gt_eq(low).and(expr.lt_eq(high)))
}
expr.clone().gt_eq(low).and(expr.lt_eq(high))
})
}

/// Visit a SQL `TRIM` function.
Expand Down Expand Up @@ -891,11 +891,11 @@ impl SQLExprVisitor<'_> {
) -> PolarsResult<Expr> {
let subquery_result = self.visit_subquery(subquery, SubqueryRestriction::SingleColumn)?;
let expr = self.visit_expr(expr)?;
if negated {
Ok(expr.is_in(subquery_result).not())
Ok(if negated {
expr.is_in(subquery_result).not()
} else {
Ok(expr.is_in(subquery_result))
}
expr.is_in(subquery_result)
})
}

/// Visit `CASE` control flow expression.
Expand Down
17 changes: 17 additions & 0 deletions py-polars/tests/unit/sql/test_joins.py
Original file line number Diff line number Diff line change
Expand Up @@ -306,3 +306,20 @@ def test_non_equi_joins(constraint: str) -> None:
LEFT JOIN tbl ON {constraint} -- not an equi-join
"""
)


def test_implicit_joins() -> None:
# no support for this yet; ensure we catch it
with pytest.raises(
SQLInterfaceError,
match=r"not currently supported .* use explicit JOIN syntax instead",
), pl.SQLContext(
{"tbl": pl.DataFrame({"a": [1, 2, 3], "b": [4, 3, 2], "c": ["x", "y", "z"]})}
) as ctx:
ctx.execute(
"""
SELECT t1.*
FROM tbl AS t1, tbl AS t2
WHERE t1.a = t2.b
"""
)

0 comments on commit 4bd0442

Please sign in to comment.