[GitHub] [arrow] Alfred-Mountfield commented on pull request #12451: ARROW-15705: [JavaScript] Allowing appending null on children in a StructBuilder

[GitBox <gi...@apache.org>]

Alfred-Mountfield commented on PR #12451:
URL: https://github.com/apache/arrow/pull/12451#issuecomment-1090579125

   Apologies @domoritz  on such a late reply, this dropped off my radar as I completely moved away from Arrow related stuff for a while. 
   
   I've tried recreating it with the following:
   ```ts
   describe ('StructBuilder', () => {
       test('Appending Null', () => {
           let list_field = new Field('n1', new FixedSizeList(2, new Field('item', new Float64())), true);
           let struct_field = new Field('foo', new Struct([list_field]), true);
   
           let builder = makeBuilder({
               type: struct_field.type,
               nullValues: [null, undefined],
           });
   
           builder.append(null);
           console.log('Struct numChildren: ' + builder.numChildren);
           console.log('Struct nullCount: ' + builder.nullCount);
           console.log('Struct length: ' + builder.length);
           console.log('Child numChildren: ' + builder.children[0].numChildren);
           console.log('Child nullCount: ' + builder.children[0].nullCount);
           console.log('Child length: ' + builder.children[0].length);
       }
   }
   ```
   
   When ran **with the change made in this PR**:
   ```
       Struct numChildren: 1
       Struct nullCount: 1
       Struct length: 1
       Child numChildren: 1
       Child nullCount: 1
       Child length: 1
   ```
   
   and when ran **without the change**:
   ```
       Struct numChildren: 1
       Struct nullCount: 1
       Struct length: 1
       Child numChildren: 1
       Child nullCount: 0
       Child length: 0
   ```
   
   As you can see, the null-count and length of the child is 0, whereas the spec seems to suggest the Children arrays should be the same length as the Struct array.
   
   I tried extending my testing with `makeVector` but I wasn't quite able to figure out the APIs in terms of how I'd represent the null data. I'm assuming using the `export function makeData<T extends Null>(props: NullDataProps<T>): Data<T>;` interface but I don't quite have capacity to investigate.
   
   As we discussed above, if these changes need to be reflected in the `UnionBuilder` or others, it'd be best in another PR.
   
   Hopefully the simple test I gave above helps with testing those, specifically we're interested in checking that children of the builders correctly have their `setValid` methods called and thus have the correct `nullCount` and `length`.


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