Skip to content

Conversation

Tmonster
Copy link
Collaborator

@Tmonster Tmonster commented Sep 3, 2025

Tests structs and primitive types. Have not yet figured out how create a map type with a NOT NULL requirement for lists and maps. I've stepped through the code though, and we should still throw an error in these cases

@Tmonster Tmonster requested a review from Tishj September 4, 2025 13:15
}
auto &not_null_constraint = constraint->Cast<NotNullConstraint>();
if (not_null_constraint.index.IsValid() && not_null_constraint.index.index == column.pos) {
required = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bit of a nitpick, so if there's no other reason to touch the PR, feel free to ignore:
this can use a break to skip the other constraints


if (partition_id.IsValid()) {
data_file.partition_spec_id = static_cast<int32_t>(partition_id.GetIndex());
if (partition_id) {
Copy link
Collaborator

@Tishj Tishj Sep 4, 2025

Choose a reason for hiding this comment

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

This if/else is redundant now

Also, I am weary of using ic_table.table_info.table_metadata.default_spec_id here, because that is taken from the rest_api_objects::TableMetadata, which has an accompanying has_default_spec_id, which I'm not sure is checked.

Though that bool could only be used for stage-create, when a spec_id is not assigned to the table yet ?
Not sure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I'm pretty sure the default spec id is required in the table metadata response, since the optional field is missing here

That was also the experience I had when creating a table, the catalog would error if no spec id was set.
We set it in IcebergCreateTableRequest::CreateTableToJSON

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but it is redundant yes

@Tmonster Tmonster merged commit 0914d90 into duckdb:main Sep 5, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants