fix: Raise error instead of panicking for unsupported SQL operations

[jqnatividad]

as its actually reachable and causes a panic when attempting unsupported ops like INSERT, UPDATE, DELETE, etc.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.63%. Comparing base (d9f3e3d) to head (319842d).
Report is 28 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #20789      +/-   ##
==========================================
- Coverage   79.78%   79.63%   -0.15%     
==========================================
  Files        1561     1571      +10     
  Lines      221982   223153    +1171     
  Branches     2531     2546      +15     
==========================================
+ Hits       177107   177719     +612     
- Misses      44293    44850     +557     
- Partials      582      584       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ritchie46]

Nice, could you add a test on the python side for this? E.g. testing you get the error you expect.

[alexander-beedie]

as its actually reachable and causes a panic when attempting unsupported ops like INSERT, UPDATE, DELETE, etc.

FYI: the unreachable() was specific to SetOperation, all the variants of which are handled in the match block (assuming Polars wasn't specially compiled without the semi_anti_join feature).

I think it's likely an artefact from an earlier version of sqlparser-rs where we had some set-op limitations, though can't recall why it was marked as unreachable instead of just falling through to the polars_bail right underneath 🤔

Anyway, you can simplify that final catch-all match branch to this now:

op => {
    polars_bail!(SQLInterface: "'{}' operation is currently unsupported", op)
},

No need for any special SetOperation handling.

[jqnatividad]

Thanks for the clarification @alexander-beedie. I was actually puzzling why it was so and was thinking of adding match arms specifically for SetExpr::Insert and SetExpr::Update instead in the enclosing match expr.

[alexander-beedie]

If you simplify as above and add a test, we can get this one merged (though does make me think the panic you mentioned must be coming from elsewhere, as UPDATE, INSERT, and DELETE aren't set ops and wouldn't have hit that unreachable! 🤔

[jqnatividad]

Yes. As I was creating the test for the fix, the error is being caused by something else other than INSERT or UPDATE as I originally suspected, as the use of both invalid operations are already being handled by execute_statement:

polars/crates/polars-sql/src/context.rs

Lines 193 to 209 in 24fccc4

	pub(crate) fn execute_statement(&mut self, stmt: &Statement) -> PolarsResult<LazyFrame> {
	let ast = stmt;
	Ok(match ast {
	Statement::Query(query) => self.execute_query(query)?,
	stmt @ Statement::ShowTables { .. } => self.execute_show_tables(stmt)?,
	stmt @ Statement::CreateTable { .. } => self.execute_create_table(stmt)?,
	stmt @ Statement::Drop {
	object_type: ObjectType::Table,
	..
	} => self.execute_drop_table(stmt)?,
	stmt @ Statement::Explain { .. } => self.execute_explain(stmt)?,
	stmt @ Statement::Truncate { .. } => self.execute_truncate_table(stmt)?,
	_ => polars_bail!(
	SQLInterface: "statement type {:?} is not supported", ast,
	),
	})
	}

If you look at dathere/qsv#2450, this is the SQL that's causing us to trigger the unreachable:

WITH u AS (
  SELECT
    small.FQDN,
    small.NS1,
    small.NS2,
    small.NS3,
    small.NS4,
    small.NS5,
    small.NS6,
    small.NS7,
    small.NS8,
    small.NS9,
    small.NS10,
    small.NS11,
    small.NS12
  FROM
    small
    INNER JOIN large ON small.FQDN = large.FQDN
)
UPDATE
  large
SET
  FQDN = u.FQDN,
  NS1 = u.NS1,
  NS2 = u.NS2,
  NS3 = u.NS3,
  NS4 = u.NS4,
  NS5 = u.NS5,
  NS6 = u.NS6,
  NS7 = u.NS7,
  NS8 = u.NS8,
  NS9 = u.NS9,
  NS10 = u.NS10,
  NS11 = u.NS11,
  NS12 = u.NS12
FROM
  u
WHERE
  large.FQDN = u.FQDN;