Fix handling dynamic table filters in RemoveUnusedColumns#18033
Conversation
Mytherin
left a comment
There was a problem hiding this comment.
Thanks for the fix! Looks good - some comments below
| PRAGMA enable_verification | ||
|
|
||
| statement ok | ||
| INSTALL tpch |
There was a problem hiding this comment.
This should be require tpch
| CALL dbgen(sf=0.1); | ||
|
|
||
| statement ok | ||
| SELECT * FROM (SELECT * FROM lineitem LIMIT 500) ORDER BY l_orderkey DESC LIMIT 10; |
There was a problem hiding this comment.
dbgen is pretty slow - can we either add this to one of the existing TPC-H tests (e.g. test/sql/tpch/tpch_topn.test_slow) or write a failing test that uses a (smaller synthetic) dataset that likely also exhibits the problem.
| auto column_ref = make_uniq<BoundColumnRefExpression>(std::move(column_type), filter_binding); | ||
| auto filter_expr = filter.second->ToExpression(*column_ref); | ||
| VisitExpression(&filter_expr); | ||
| if (filter_expr->GetExpressionClass() == ExpressionClass::BOUND_CONSTANT) { |
There was a problem hiding this comment.
Can we use filter_expr->IsScalar() instead?
| VisitExpression(&filter_expr); | ||
| if (filter_expr->GetExpressionClass() == ExpressionClass::BOUND_CONSTANT) { | ||
| AddBinding(*column_ref); | ||
| column_refs.push_back(std::move(column_ref)); |
There was a problem hiding this comment.
Instead of having the special code here - can we just set filter_expr = std::move(column_ref)? Then we don't need the extra column_refs vector either.
|
Can we also retarget this to |
|
Thanks! |
Fix handling dynamic table filters in RemoveUnusedColumns (duckdb/duckdb#18033)
Fix handling dynamic table filters in RemoveUnusedColumns (duckdb/duckdb#18033) Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
This PR fixes #18028. ToExpression of dynamic table filter may return constant true expression which causes the column referenced in the filter to be lost.