docs: clarify nullability information source for empty literals

[vbarua]

Looking at the oneof literal_type block I noticed that the empty_list variant and empty_map variant reuse a Type message, much like the null variant.

      Type null = 29; // a typed null literal
      ...
      Type.List empty_list = 31;
      Type.Map empty_map = 32;

Like Type, Type.List and Type.Map also declare their nullability directly. To avoid ambiguity, I updated the docs to specify that the nullability of these literals should come from the inner message here, and not from the nullable field of the Literal message.

[github-actions]

ACTION NEEDED

Substrait follows the Conventional Commits
specification for
release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

[EpsilonPrime]

I need to verify but the text format appears to set both for empty lists and just the type for empty maps (based on its test cases taken from Isthmus).

[vbarua]

@EpsilonPrime where you able to verify this?

[EpsilonPrime]

It shouldn't matter going forward, I will make sure that the text format follows this guideline by adding some tests.

[EpsilonPrime]

For empty lists the following behavior is present for the text format:

{}_list<string> -> nothing nullable
{}_list<string?> -> literal is nullable, list is not nullable
{}_list?<string> -> list is nullable, literal is not
{}_list?<string?> -> both are nullable

It seems like this nullability covers the nullability of the list even if it's empty. Even if it's empty is it legal to have a nullable version of it?

[EpsilonPrime]

Added the empty_map tests in substrait-io/substrait-cpp#97.

[EpsilonPrime]

The text format is definitely using this nullability over the one in the type. It's the same behavior it uses for non-empty lists and maps.

[westonpace]

Thanks for removing the ambiguity

[westonpace]

This wording is ok. I have a suggestion for something slightly different if we want.

[vbarua]

I want to double check something before merging this, will look at it this week.

[vbarua]

I want to double check something before merging this, will look at it this week.

Weston's suggestion clarified this for me. I think this should be good as is with his changes incorporated.

[EpsilonPrime]

Will check this on Thursday.

[EpsilonPrime]

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

[vbarua]

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.

[EpsilonPrime]

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

[westonpace]

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

[EpsilonPrime]

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).