Support setting key field in MapBuilder #7101
Conversation
| /// field's data type does not match that of `K` | ||
| pub fn with_keys_field(self, field: impl Into<FieldRef>) -> Self { | ||
| let field: FieldRef = field.into(); | ||
| assert!(!field.is_nullable(), "Key field must not be nullable"); |
There was a problem hiding this comment.
Should this return Err instead? Given it's user input
There was a problem hiding this comment.
Updated.
I'm thinking now this check might be unnecessary because finish asserts here that there are no null-keys anyway.
There was a problem hiding this comment.
Yeah, I've removed checking for nullability of the key field. I didn't love that MapBuilder::with_keys_field returns a Result while MapBuilder::with_values_field or GenericListBuilder::with_field do not. We still ensure that we protect against nulls with the existing assertion in finish.
Happy to go back to returning Result. Not feeling strongly about this.
There was a problem hiding this comment.
I think we still need this check; the check in finish() only checks for null values, but the datatype of the key field itself should always be non-nullable. An array could have no null values but still a datatype with nullable = true I believe.
I can see how it's awkward for with_keys_field to be the only one returning Result, but it makes sense to me since it has a pre-condition to guard against, and we should take advantage of Rust's type system where possible instead of documenting panics and hoping users read the documentation.
There was a problem hiding this comment.
Given a nullable map field is simply out of spec, I personally don't think panicking is wrong and tbh for this sort of invariant it is arguably more correct...
I don't fell strongly but panicking would be more consistent with the rest of the builders
0cd50a2 to
53bbbfc
Compare
| /// By default a nullable field is created with the name `values` | ||
| /// By default, a nullable field is created with the name `keys` | ||
| /// | ||
| /// Returns an error if the given field is nullable. |
There was a problem hiding this comment.
I think this needs to be updated
| /// field's data type does not match that of `K` | ||
| pub fn with_keys_field(self, field: impl Into<FieldRef>) -> Self { | ||
| let field: FieldRef = field.into(); | ||
| assert!(!field.is_nullable(), "Keys field must not be nullable"); |
There was a problem hiding this comment.
We could also move this check into finish, but I don't think it really matters
| ) | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Just for sanity could we get a test that it panics if you try to specify a keys field with the wrong datatype as well
| /// Panics if the given field is nullable as map keys are not allowed to be null | ||
| /// | ||
| /// Note: [`Self::finish`] and [`Self::finish_cloned`] will panic if the | ||
| /// field's data type does not match that of `K` |
There was a problem hiding this comment.
| /// Panics if the given field is nullable as map keys are not allowed to be null | |
| /// | |
| /// Note: [`Self::finish`] and [`Self::finish_cloned`] will panic if the | |
| /// field's data type does not match that of `K` | |
| /// Note: [`Self::finish`] and [`Self::finish_cloned`] will panic if the | |
| /// field's data type does not match that of `K` or the field is nullable |
|
Thank you for catching that |
Closes #7100 by allowing overrides of the key field on
MapBuilderthrough a new method calledMapBuilder::with_key_field.The goal of this is to support setting field metadata which, e.g.,
iceberg-rustwould use to assign Iceberg's field ids, see apache/iceberg-rust#863 (comment).Linking the
with_value_fieldPR for reference: #5483