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

Jump to bottom

docs: clarify nullability information source for empty literals #602

Merged
merged 1 commit into from
Apr 18, 2024
Merged

docs: clarify nullability information source for empty literals #602

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions proto/substrait/algebra.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,9 +825,11 @@ message Expression {
UserDefined user_defined = 33;
}

// whether the literal type should be treated as a nullable type. Applies to
// all members of union other than the Typed null (which should directly
// declare nullability).
// Whether the literal_type above should be treated as a nullable type.
// Applies to all members of the literal_type oneof EXCEPT:
// * Type null (must be nullable by definition)
// * Type.List empty_list (use Type.List::nullability)
Copy link
Member

Choose a reason for hiding this comment

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

If the Type.List::nullability is the nullability of the type inside the list then I don't think this applies.

Copy link
Member Author

@vbarua vbarua Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

substrait/proto/substrait/type.proto

Lines 186 to 190 in 1a51b3d

message List {
Type type = 1;
uint32 type_variation_reference = 2;
Nullability nullability = 3;
}

I believe that Type.List::nullability is the nullability of the list itself, and Type.List::type::nullability is the nullability of the internal type.

Copy link
Member

Choose a reason for hiding this comment

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

That matches what I believe. But for empty lists and empty maps why is that any different than populated lists and maps?

Copy link
Member

Choose a reason for hiding this comment

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

Empty lists and empty maps have two spots for list nullability (so we need to pick one to be authoritative):

// List nullability (authoritative)
Literal::empty_list::list::nullability
// Also list nullability (redundant)
Literal::nullability

Populated lists and maps have one spot for list nullability and one spot for item nullability:

// Applies to the item
Literal::list::values...nullability
// Applies to the list (only option, thus authoritative)
Literal::nullability

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I see it now. An alternative would be to change the type of of empty_list and empty_map to Type but that doesn't work simply for maps (which would need two nullability values for its key and type).

// * Type.Map empty_map (use Type.Map::nullability)
bool nullable = 50;

// optionally points to a type_variation_anchor defined in this plan.
Expand Down
Loading