Re: [I] UnionBuilder produces incorrect Union DataType [arrow-rs]

["gstvg (via GitHub)" <gi...@apache.org>]

gstvg commented on issue #1637:
URL: https://github.com/apache/arrow-rs/issues/1637#issuecomment-2319003403

   I also ran into this on #6303
   We probably should care about the generated type_id of each field too.
   
   Currently, the returned union only contains fields appended at least once. So every `build_union` calls below returns a different set of children with different type_id each.
   
   ```
           enum Value {
               U64(u64),
               I32(i32),
           }
   
           fn build_union(values: &[Value]) -> UnionArray {
               let mut builder = UnionBuilder::new_sparse();
   
               for value in values {
                   match value {
                       Value::U64(u) => builder.append::<UInt64Type>("u64", *u).unwrap(),
                       Value::I32(i) => builder.append::<Int32Type>("i32", *i).unwrap(),
                   };
               }
   
               builder.build().unwrap()
           }
   
           build_union(&[Value::U64(10), Value::I32(-4)]); // u64=0 i32=1
           build_union(&[Value::U64(10), Value::U64(40)]); // u64=0
           build_union(&[Value::I32(-9), Value::I32(-4)]); // i32=0 u64=1
   ```
   
   Do you think we should follow `GenericListBuilder`, and add a method to specify `UnionFields`, or require it as an argument for `UnionBuilder` constructors?
   
   If the fields are somehow specified, should the returned array contains every child specified, or only those appended at least one time?
   
   I thought about another option, and would like to know what you think 
   
   First, we need to add an id argument to the current `append*` functions:
   
   ```
   fn append(&mut self, type_name: &str, id: i8, value: T::Native)
   fn append_null(&mut self, type_name: &str, id: i8) 
   ```
   
   And then add
   
   ```
   fn append_nullable(&mut self, type_name: &str, id:i8, value: Option<T::Native>)
   ```
   
   And finally, document that nullable fields should only use `append_null` and `append_nullable`, while non nullable ones should only use `append`, and return an error if the same field is appended with mixed calls
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org