Conversation
|
Preparation for DuckDB 1.2 release, should be merged once version is officially released with extensions |
src/scan/postgres_scan.cpp
Outdated
There was a problem hiding this comment.
Maybe interesting to note that it is a PG table?
| result["Table"] = GetRelationName(bind_data.rel); | |
| result["Table"] = GetRelationName(bind_data.rel) + " (PG)"; |
There was a problem hiding this comment.
Explain already show it is POSTGRES_SCAN node. We can add it if needed.
There was a problem hiding this comment.
It doesn't show that for explain analyze for some reason I think.
There was a problem hiding this comment.
src/scan/postgres_scan.cpp
Outdated
There was a problem hiding this comment.
This seems like a bit of a hack. Do we really need this? If so, can you add a comment explaining why?
Also, is this actually correct? Because now we don't actually filter anything for these filter types inside postgres, but duckdb will not filter on that side either. I feel like we probably need some tests that show that we return correct result when using these kind of things.
There was a problem hiding this comment.
Updated construction of pushdown filters. Looking at duckdb code STRUCT_EXTRACT is only created if struct_extract is used in query, while DYNAMIC_FILTER is constructed when there is topN node (LIMIT). Both of these should not matter to filtering.
src/scan/postgres_scan.cpp
Outdated
There was a problem hiding this comment.
Why do we return 0 here instead of throwing an error? Do we have a query that triggers this?
There was a problem hiding this comment.
DYNAMIC_QUERY happens when there is LIMIT in query so we'll see this filter pushed down to scan, but we can't do anything about it except to skip it.
There was a problem hiding this comment.
Alright, I looked a bit closer at the code and understand much better what all these things are: I think a somewhat cleaner/future-proof way of handling this is by remembering if we're in an optional filter or not (probably by passing it as a third argument to the recursive function). Then for all these filters that we don't support, we do nothing if it's inside an optional filter, or throw an error if it's not inside an optional filter.
src/scan/postgres_scan.cpp
Outdated
| } | ||
| /* DYNAMIC_FILTER is push down filter from topN execution */ | ||
| case duckdb::TableFilterType::DYNAMIC_FILTER: | ||
| return 0; |
There was a problem hiding this comment.
Now it should be possible to remove this line right?
| return 0; |
src/scan/postgres_scan.cpp
Outdated
|
|
||
| int | ||
| PostgresScanGlobalState::ExtractQueryFilters(duckdb::TableFilter *filter, const char *column_name, | ||
| duckdb::string &query_filters, bool is_optional_filter_parent) { |
There was a problem hiding this comment.
nit: "parent" seems like the wrong naming imo. Maybe one of these names instead:
| duckdb::string &query_filters, bool is_optional_filter_parent) { | |
| duckdb::string &query_filters, bool is_optional_filter_child) { |
| duckdb::string &query_filters, bool is_optional_filter_parent) { | |
| duckdb::string &query_filters, bool is_inside_optional_filter) { |
Enabled and fixed tests
3 participants
No description provided.