Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duckdb 1.2 update #548

Merged
merged 26 commits into from
Feb 6, 2025
+174 −99
Merged

Duckdb 1.2 update #548

Changes from 1 commit
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
acda61a
Update duckdb to ib6513473ba48f46c39c8a9d506e4ad32b7a6f2ae commmit
mkaruza Jan 20, 2025
a2f32a6
Update to DuckDB 1.2
mkaruza Jan 20, 2025
15d5300
Update to DuckDB 1.2 and fix regression tests
mkaruza Jan 20, 2025
09986a1
Disable regression test that work with extensions
mkaruza Jan 20, 2025
3ae7f5f
Formating
mkaruza Jan 20, 2025
9556f29
pytest fix
mkaruza Jan 20, 2025
b16aed9
Iterate recursively push down filter and construct them
mkaruza Jan 23, 2025
912d09f
Update duckdb to `V1.2 histrionicus` commit
mkaruza Jan 23, 2025
f556695
Update DUCKDB_VERSION
mkaruza Jan 23, 2025
c0b9a32
Don't throw if unsupported filter is optional filter
mkaruza Jan 27, 2025
9abe5b4
Formating
mkaruza Jan 27, 2025
b912069
Missing parenthesis so formating back
mkaruza Jan 27, 2025
f075a41
Add DYNAMIC_FILTER and handle as switch in other unsupported filter
mkaruza Jan 27, 2025
22d3048
Merge remote-tracking branch 'origin' into duckdb-1.2
JelteF Feb 4, 2025
264da7a
Rename to is_inside_optional_filter
JelteF Feb 4, 2025
2e0597a
Correctly pass on is_inside_optional_filter
JelteF Feb 4, 2025
9c2a42b
Update duckdb to latest v1.2-histrionicus branch commit
JelteF Feb 4, 2025
8030edd
Downgrade to latest nightly build
JelteF Feb 4, 2025
f47c105
Disable tests that fail due to extensions not being available
JelteF Feb 4, 2025
47eef96
Update to official v1.2.0 release
JelteF Feb 5, 2025
3aa062f
Enable extension tests
JelteF Feb 5, 2025
5be0124
Updated to Duckdb 1.2 TAG commit
mkaruza Feb 6, 2025
b1c162e
Only format C/C++ files with clang-format
JelteF Feb 6, 2025
0889841
Make clang-format happy
JelteF Feb 6, 2025
fa5f9ec
Update non_superuser test output for 1.2.0
JelteF Feb 6, 2025
753b0ef
Use duckdgq as an example community extension, avro is not released f…
JelteF Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Formating
@mkaruza
mkaruza committed Jan 27, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
commit 3ae7f5f93f423607b07bdf6b52cbf4ae4a667afb
36 changes: 18 additions & 18 deletions src/scan/postgres_scan.cpp
View file
Original file line number Diff line number Diff line change
@@ -18,23 +18,23 @@ namespace pgduckdb {
void
PostgresScanGlobalState::ConstructQueryFilter(duckdb::TableFilter *filter, const char *column_name) {
switch (filter->filter_type) {
case duckdb::TableFilterType::CONSTANT_COMPARISON:
case duckdb::TableFilterType::IS_NULL:
case duckdb::TableFilterType::IS_NOT_NULL:
case duckdb::TableFilterType::CONJUNCTION_OR:
case duckdb::TableFilterType::CONJUNCTION_AND:
case duckdb::TableFilterType::IN_FILTER:
scan_query << filter->ToString(column_name).c_str();
break;
case duckdb::TableFilterType::OPTIONAL_FILTER: {
auto optional_filter = reinterpret_cast<duckdb::OptionalFilter*>(filter);
ConstructQueryFilter(optional_filter->child_filter.get(), column_name);
break;
}
case duckdb::TableFilterType::STRUCT_EXTRACT:
case duckdb::TableFilterType::DYNAMIC_FILTER:
scan_query << "1 = 1";
break;
case duckdb::TableFilterType::CONSTANT_COMPARISON:
case duckdb::TableFilterType::IS_NULL:
case duckdb::TableFilterType::IS_NOT_NULL:
case duckdb::TableFilterType::CONJUNCTION_OR:
case duckdb::TableFilterType::CONJUNCTION_AND:
case duckdb::TableFilterType::IN_FILTER:
scan_query << filter->ToString(column_name).c_str();
break;
case duckdb::TableFilterType::OPTIONAL_FILTER: {
auto optional_filter = reinterpret_cast<duckdb::OptionalFilter *>(filter);
ConstructQueryFilter(optional_filter->child_filter.get(), column_name);
break;
}
case duckdb::TableFilterType::STRUCT_EXTRACT:
case duckdb::TableFilterType::DYNAMIC_FILTER:
scan_query << "1 = 1";
Copy link
Collaborator

@JelteF JelteF Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

break;
}
}

@@ -186,7 +186,7 @@ duckdb::InsertionOrderPreservingMap<duckdb::string>
PostgresScanTableFunction::ToString(duckdb::TableFunctionToStringInput &input) {
auto &bind_data = input.bind_data->Cast<PostgresScanFunctionData>();
duckdb::InsertionOrderPreservingMap<duckdb::string> result;
result["Table"] = GetRelationName(bind_data.rel);
result["Table"] = GetRelationName(bind_data.rel);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe interesting to note that it is a PG table?

Suggested change
result["Table"] = GetRelationName(bind_data.rel);
result["Table"] = GetRelationName(bind_data.rel) + " (PG)";

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain already show it is POSTGRES_SCAN node. We can add it if needed.

Copy link
Collaborator

@JelteF JelteF Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't show that for explain analyze for some reason I think.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah cf. this diff https://github.com/duckdb/pg_duckdb/pull/548/files#diff-dd9a38fc1644731fee36f5adfd11ba162ed425a9f3d2a90205d1ae5470288826L18

return result;
}