search

duckdb / duckdb_spatial

MIT License
492 stars 41 forks source link

Pull up pushed down filters from table scan before replacing with index scan #429

Closed Maxxen closed 1 month ago

Maxxen commented 1 month ago

Closes #428

~This is a bit of a band-aide fix that disabled index scans if the table already has normal filters pushed down. I think its generally expected that the index scan would be way more selective, but as we don't have the chance to swap in the index scan until after normal filters are already pushed down we can't really apply it instead.~

~I made a quick attempt at trying to reconstruct and "pull up" the filters from the table scan again but it seems like it would be very fragile and error prone to do correctly 100% of the time, especially for more advanced queries. It's also a bit silly as we'd basically undo a lot of the work that went into pushing down the table filters to begin with and have to replace basically the whole logical get node.~

~The proper solution would be to improve the index interface in DuckDB core to give custom indexes a chance to properly participate in the query planning process while filters are being evaluated for pushdown. We could then also look at stats/estimated cardinalities to decide whether or not to use an index scan or not, instead of just always matching specific predicates.~

We now pull up the filters from the table scan before replacing it with an index scan, ensuring index scans are applied even if other filters are present as well. This is usually the right thing to do as geometry predicates are usually much more expensive to evaluate compared to other filters that DuckDB can push down.

youngpm commented 1 month ago

Awesome, thanks for the quick fix! Guessing this will show up in v1.1.3?

Maxxen commented 1 month ago

We're most likely going to hot-swap and update the extension we distribute for 1.1.2 tomorrow or early next week, so a FORCE INSTALL spatial should be all that's needed to get this fix once that's done. I just have another in-progress PR id like to sneak in first.

youngpm commented 3 weeks ago

@Maxxen did this go out yet? Just tried the force install but no luck!

Maxxen commented 3 weeks ago

@youngpm Sorry we didn't bump the extension for v1.1.2, but this fix should be available in the recently released v1.1.3!

youngpm commented 3 weeks ago

Ah thank you, got it!