divyankshah commented on PR #5235: URL: https://github.com/apache/datafusion-comet/pull/5235#issuecomment-5721332768
Thanks @sunchao, that is a good catch and the diagnosis is exactly right. Fixed in 90dc9a36. The guard was only applied at the call sites in `arrays_overlap_list_generic`, so once the top level check passed, every child was rebuilt whether or not it held float data. An empty struct child then reached `StructArray::new`, which cannot infer a length from zero columns. Your `_partition` example is a real reachable path for that, and I had not considered a zero column struct at all. The fix is the recursive guard you suggested. `has_float_leaf` now runs inside `normalize_nested_floats` rather than only at the call sites, so any subtree without a float leaf is returned unchanged. Empty structs fall out of that naturally since they have no float leaf, and as a side benefit the rebuild is now proportional to the float data instead of the whole element. Three regression tests added, all verified to fail without the guard and pass with it: - `test_struct_with_empty_struct_field_overlap` in `arrays_overlap.rs`, which mirrors your query shape with two rows of ordinary doubles and an empty struct field. Without the guard it panics in `struct_array.rs`. - `test_normalize_struct_with_empty_struct_sibling`, the same case at the helper level. - `test_normalize_leaves_float_free_subtree_untouched`, which pins the no rebuild behaviour using a nested float free struct child. Also rebased onto current main while I was in here. Local results after the fix: - `cargo test -p datafusion-comet-spark-expr`: 862 passed, 0 failed - `cargo clippy -p datafusion-comet-spark-expr --lib --all-targets -- -D warnings`: clean - `cargo fmt -p datafusion-comet-spark-expr -- --check`: clean I have not been able to run the Iceberg case end to end locally, so if you have that setup handy I would appreciate a confirmation on the original repro. @andygrove, thanks for the approval and for digging back into the `typeWithProperEquals` split rather than taking the earlier round on trust. Since the head has moved, this will need another look before it goes in. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
