Add Iceberg View support for REST catalogs#887
Conversation
1a92243 to
3c0c883
Compare
|
Updated: fixed clang-format violations and force-pushed. |
|
Hi, I've tested a lot this yesterday and I'm adding a small fix for a deadlock in ScanViews. When views exist in the REST catalog but aren't cached locally ScanViews held entry_lock and called GetViewEntry which tried to acquire the same lock. I've split it into a locking wrapper + internal method. Includes a regression test that hangs without the fix and passes with it. |
|
Hi @LucaSoato, Thanks for the PR! This is a pretty big feature, but we will take a look soon. Unfortunately, this is a bit too big of a feature to merge for the v1.5.2 release. We will try to keep main compatible with v1.5.2 so you can install a nightly binary to use this feature if/when we merge |
|
Hi @Tmonster, |
Tmonster
left a comment
There was a problem hiding this comment.
Thanks! A couple of mostly minor comments.
Could you also retarget this to v1.5-variegata? That way we can try and get it in for v1.5.3
| auto entry_name = info.name; | ||
|
|
||
| // Handle VIEW_ENTRY drops | ||
| if (info.type == CatalogType::VIEW_ENTRY) { |
There was a problem hiding this comment.
nit: can we get a switch statement here for different CatalogTypes? The default case can just throw a NotImplementedException
There was a problem hiding this comment.
There was a problem hiding this comment.
nit: Can we check cascade before the View type? I think cascade is not even an Iceberg compatible keywork
There was a problem hiding this comment.
| auto view_key = IcebergTableInformation::GetTableKey(namespace_items, info.view_name); | ||
|
|
||
| // Workaround: CreateViewInfo::Copy() does not copy `names`, so we patch it manually. | ||
| // See: https://github.com/duckdb/duckdb/pull/21817 |
There was a problem hiding this comment.
Looks like duckdb/duckdb#21819 was merged, so the workaround can be removed?
There was a problem hiding this comment.
| auto repr_obj = yyjson_mut_arr_add_obj(doc, repr_arr); | ||
| yyjson_mut_obj_add_strcpy(doc, repr_obj, "type", "sql"); | ||
| yyjson_mut_obj_add_strcpy(doc, repr_obj, "sql", view_sql.c_str()); | ||
| yyjson_mut_obj_add_strcpy(doc, repr_obj, "dialect", "duckdb"); |
There was a problem hiding this comment.
duckdb or DuckDB or does it not matter?
There was a problem hiding this comment.
I've used the lowercase and saved it to the constant IcebergConstants::ViewDuckDBDialect
https://github.com/LucaSoato/duckdb-iceberg/blob/db61a52199e69e2f2a900693752b374007342db6/src/include/common/iceberg_constants.hpp#L17-L19
In the Iceberg View examples, I saw them always using lowercase.
| // commit updates to multiple tables in an atomic transaction | ||
| supported_urls.insert("POST /v1/{prefix}/transactions/commit"); | ||
| // list all view identifiers | ||
| supported_urls.insert("GET /v1/{prefix}/namespaces/{namespace}/views"); |
There was a problem hiding this comment.
Views are technically not in the list of default supported endpoints, so I think this should be removed
There was a problem hiding this comment.
Removed the rows, I've left a comment so if anyone looks into this knows why the endpoints aren't included.
| } | ||
|
|
||
| // Check if the view endpoint is supported | ||
| if (ic_catalog.supported_urls.find("GET /v1/{prefix}/namespaces/{namespace}/views/{view}") == |
There was a problem hiding this comment.
Should we do this check first? Any view that we create that we later call CreateViewEntryInternal will have to pass this check right?
There was a problem hiding this comment.
So right now we check:
- if it's in transaction.deleted_views
- if it's in view_catalog_entries cache
- if we created it in transaction.created_views
- and only then if views are in supported urls
Technically it's right because the first three operations are in memory and do not require URLs. The thing is that if in the same transaction we try to create a view, referencing the same view later, it will fail anyway but only during the commit of such operation for catalogs that don't support views.
If you want we can add a POST /v1/{prefix}/namespaces/{namespace}/views endpoint check at the top of CreateView so it fails immediately on no-views catalogs?
| try { | ||
| load_result = IRCAPI::GetView(context, ic_catalog, schema, view_name); | ||
| } catch (...) { | ||
| return nullptr; |
There was a problem hiding this comment.
nit: Can we not have a naked catch here? Not a fan of those when it comes to debugging
There was a problem hiding this comment.
I've improved this part by refactoring IRCAPI::GetView to return APIResult<unique_ptr<const LoadViewResult>>, similarly to what happens in GetTable
| if (!repr.has_sqlview_representation) { | ||
| continue; | ||
| } | ||
| if (repr.sqlview_representation.dialect == "duckdb") { |
There was a problem hiding this comment.
I think we should hardcode the "duckdb" string somewhere. Maybe in include/common/IcebergConstants.hpp?
There was a problem hiding this comment.
| if (view_sql.empty()) { | ||
| for (auto &repr : version.representations) { | ||
| if (repr.has_sqlview_representation) { | ||
| view_sql = repr.sqlview_representation.sql; |
There was a problem hiding this comment.
Can we also log a warning that no DuckDB dialect has been found?
a search for DUCKDB_LOG_WARNING should reveal some examples
There was a problem hiding this comment.
|
Thanks so much for your feedback, I'll get back to this asap 🚀 |
f4a95bd to
267f371
Compare
|
@LucaSoato can we have a support for user defined dialect value (may be from a variable/global), some Kond of override functionality. That way user can try to read multi dialects. |
@Tmonster what do you think about it? |
Tmonster
left a comment
There was a problem hiding this comment.
Thanks for the updates.
With a more thorough review, I think we are still missing some testing/functionality that I would appreciate having in this PR
There are no tests on the unhappy path. Can we add some?
It would be nice to see
- Creating a view that selects from table columns that do not exist. (I would expect this to error on view creation similar to how it does when operating on duckdb tables)
- Selecting a column from a view where the column does not exist in the view.
Other tests I would like to see
- A view that does
select * - A view that does
select * exclude - A view that joins tables
Also can we test how iceberg views are handled when we call
from duckdb_views();
from duckdb_tables();
show all tables;
I don't expect full Iceberg view functionality integrated here just yet, I just want to make sure DuckDB doesn't crash in this scenario.
One more piece of functionality I would like to see, is a table function called iceberg_view_metadata which returns the result of a call to the LoadView API. That can make view discovery on a Rest catalog a bit easier. We have a similar function for tables now, iceberg_load_table_response, you can use that as a template, we put a lot of the JSON into a variant.
I also like the idea of a preferred SQL dialect. Maybe that is something we can add to the view properties? I see in the LoadViewResult we can add properties to a view. I think that might be out of scope for this PR though, and can be added in a follow up. A config option would also work. DuckDB v2 will use the PEG parser, so we should be able to extend our parser support, and then we can have a statement like SELECT * FROM iceberg_view WITH ('sql'='duckdb), but that will obvs come much later.
| drop_info.name = info.view_name; | ||
| drop_info.cascade = false; | ||
| drop_info.if_not_found = OnEntryNotFound::RETURN_NULL; | ||
| DropEntry(context, drop_info, false); |
There was a problem hiding this comment.
This will drop the entry regardless of if he transaction succeeds right?
Currently DuckDB-Iceberg doesn't support CREATE OR REPLACE for tables because of this scenario. I would like to stay consistent with that if possible
| statement error | ||
| select * from test_view_v1; | ||
| ---- | ||
| does not exist |
There was a problem hiding this comment.
nit: <REGEX>:.*does not exist.* is what we normally use
| } | ||
| } | ||
|
|
||
| // Generate default column names if the caller gave us types but no names. |
There was a problem hiding this comment.
Can we do some more work here and potentially bind the sql query to make sure it is valid?
There was a problem hiding this comment.
Hi @Tmonster,
I think that this already happens here:
https://github.com/duckdb/duckdb/blob/c028854da4348b4d32731e460b75973364f79b74/src/planner/binder/statement/bind_create.cpp#L204
|
Hi @LucaSoato is this still something you still want merged, otherwise we can close to reduce clutter? |
|
Hi everyone, |
- DropEntry: use switch on CatalogType, default throws NotImplementedException - DropEntry: move cascade check above type switch (cascade is not in the Iceberg REST spec) - CreateView: remove CreateViewInfo::Copy() names workaround (fixed by duckdb#21819) - Hoist "duckdb" dialect and "sql" representation type into IcebergConstants - Log a warning when falling back from DuckDB dialect to another dialect on view load Also set drop_info.type = TABLE_ENTRY in the internal DoTableRename and DoTableDeletes callers so the new switch default doesn't reject them.
- add iceberg_view_metadata table function (mirrors iceberg_load_table_response) - reject CREATE OR REPLACE for views, consistent with CreateTable / HandleCreateConflict - add tests: unhappy paths (bad column/table at create), select *, select * exclude, joins - test that duckdb_views() / duckdb_tables() / show all tables do not crash with iceberg views - use <REGEX> error matchers and fix the stale test name header
27f4aeb to
686885f
Compare
|
Hi @Tmonster, thanks for the patience! Just a small recap regarding the changes:
Related to the preferred SQL dialect, it's a nice follow-up, let's leave it out of this PR and we can add it later. Thanks again and, as always, let me know if there's anything else left for this PR. |
|
p.s. |
|
(a small format fix is coming) |
|
Thanks @LucaSoato, will get to reviewing, It hurts me to say this, but we are going to limit new features to v1.5. Would you mind rebasing and retargeting |
|
It's alright, will rebase to main. |
| throw BinderException("Cannot create view in Iceberg without a query"); | ||
| } | ||
| auto &context = transaction.GetContext(); | ||
|
|
There was a problem hiding this comment.
We should check that binding_mode is set to "bind on create":
//! Whether or not to bind the view on create
CreateViewBindingMode binding_mode = CreateViewBindingMode::BIND_ON_CREATE;Since we don't want to submit to the Catalog without verifying correctness
Summary
Implements CREATE VIEW, DROP VIEW, and view loading/querying for Iceberg REST catalogs, following the Iceberg View Spec.
"duckdb"per the spec's multi-dialect representation model"duckdb"dialect, falls back to any available SQL representationCloses #314
Known workaround
CreateViewInfo::Copy()in DuckDB core does not copy thenamesfield. We patch it after copying. Fix submitted upstream: duckdb/duckdb#21817Test plan
irc_any_catalogtest suite passes (169 cases, 83,661 assertions, 0 regressions)