[BUG] Add over clause in read_sql percentile reads #3094
Conversation
CodSpeed Performance ReportMerging #3094 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3094 +/- ##
==========================================
+ Coverage 78.35% 78.64% +0.29%
==========================================
Files 611 616 +5
Lines 72517 73140 +623
==========================================
+ Hits 56819 57522 +703
+ Misses 15698 15618 -80
|
| @@ -990,21 +990,9 @@ impl Expr { | |||
| to_sql_inner(inner, buffer)?; | |||
| write!(buffer, ") IS NOT NULL") | |||
| } | |||
| Expr::IfElse { | |||
There was a problem hiding this comment.
SQL server only accepts values in case when then clauses, so it was failing on the if_else pushdowns test, because we test with conditional expressions inside the if_else branches.
I think it's better to just remove it, the benefit of having if_else pushdowns in read_sql is probably not much at all, and i'd instead rather just avoid having this be a bug in the future.
| # Skip invalid comparisons for bool_col | ||
| if column == "bool_col" and operator not in ("=", "!="): | ||
| pytest.skip(f"Operator {operator} not valid for bool_col") |
There was a problem hiding this comment.
SQL server only accepts equality based comparisons on bools, e.g. bool_col = True or bool_col != True.
In fact, we shouldn't even need to test comparisons on bools other than = and != anyway
There was a problem hiding this comment.
Nit for the test file, not necessarily for this PR. But not-equal is typically <> in sql, so maybe we should test that too.
| container_name: azuresqledge | ||
| environment: | ||
| ACCEPT_EULA: "Y" | ||
| MSSQL_SA_PASSWORD: "StrongPassword!" |
| # Skip invalid comparisons for bool_col | ||
| if column == "bool_col" and operator not in ("=", "!="): | ||
| pytest.skip(f"Operator {operator} not valid for bool_col") |
There was a problem hiding this comment.
Nit for the test file, not necessarily for this PR. But not-equal is typically <> in sql, so maybe we should test that too.
Addresses: #3075
SQL server requires an
OVERclause to be specified in percentile queries (because it's a window function). Read sql uses percentiles to determine partition bounds.Adds AzureSqlEdge as a test database. Might as well since a lot of ppl use us to read sqlserver, and have had bugs with sql server. Kind of a pain to get it set up since it requires odbc and drivers etc. but it works. It's also not much of a hit on CI times, installing drivers takes around ~15s and the extra tests take around 5s.
Additionally made some modifications to some tests and pushdowns, left comments on the rationale.