Fix approx_count_distinct for queries without a FROM #524
Conversation
I noticed two issues with the new `approx_count_distinct` implementation: 1. If no FROM clause was used it was not possible to use it 2. It would not be detected correctly as duckdb-only without `duckdb.force_execution = true` (or some other mechanism). This fixes both of those issues.
src/pgduckdb_hooks.cpp
Outdated
| * | ||
| * If there's no rtable, we're only selecting constants. From a performance | ||
| * perspective there's not really a point in using DuckDB. If we remove | ||
| * this heck many common queries that are used to inspect postgres will |
There was a problem hiding this comment.
| * this heck many common queries that are used to inspect postgres will | |
| * this hack many common queries that are used to inspect postgres will |
src/pgduckdb_hooks.cpp
Outdated
| */ | ||
| if (!query->rtable) { | ||
| if (!query->rtable && !throw_error) { |
There was a problem hiding this comment.
This looks odd - if !query->rtable we should stop the function here (otherwise ContainsCatalogTable and subsequent calls would fail pretty badly no?)
Shouldn't it be something like
if (!query->rtable) {
if (throw_error) {
elog(...)
src/pgduckdb_hooks.cpp
Outdated
| * still executing this in DuckDB. | ||
| */ | ||
| static bool | ||
| ContainsFromClause(Query *query, bool throw_error = false) { |
There was a problem hiding this comment.
Do we really want to keep throw_error?
There was a problem hiding this comment.
nope, copy paste mistake. Fixed now.
| @@ -197,7 +216,7 @@ DuckdbPlannerHook_Cpp(Query *parse, const char *query_string, int cursor_options | |||
| IsAllowedStatement(parse, true); | |||
|
|
|||
| return DuckdbPlanNode(parse, query_string, cursor_options, bound_params, true); | |||
| } else if (duckdb_force_execution && IsAllowedStatement(parse)) { | |||
| } else if (duckdb_force_execution && IsAllowedStatement(parse) && ContainsFromClause(parse)) { | |||
There was a problem hiding this comment.
You want to move ContainsFromClause(parse) before IsAllowedStatement(parse) no? o/w we could hit the same issue calling ContainsCatalogTable w/ a nullptr no?
There was a problem hiding this comment.
It's fine to call ContainsCatalogTable and ContainsPartitionedTable with a nullptr. They both call foreach_node on that pointer, and the nullptr is a valid (and I think the only valid) representation of an empty List.
I noticed two issues with the new `approx_count_distinct` implementation: 1. If no FROM clause was used it was not possible to use it 2. It would not be detected correctly as duckdb-only without `duckdb.force_execution = true` (or some other mechanism). This fixes both of those issues. Related to duckdb#499
I noticed two issues with the new
approx_count_distinctimplementation:duckdb.force_execution = true(or some other mechanism). Thisfixes both of those issues.
Related to #499