andygrove commented on PR #26266: URL: https://github.com/apache/datafusion/pull/26266#issuecomment-6220572953
Thanks for the PR. I reviewed this by reading the diff and surrounding code. I did not build or run it, so the failure scenarios below are from code reading and not reproduced. **Main concerns** 1. **Unbounded output (`stack.rs` ~L514).** `parse_n` accepts any `n` up to `i32::MAX`, and the only guard is that the i32 offsets fit. `SELECT stack(2000000000, 1)` looks like it would allocate roughly 16 GB for an Int64 column, plus a large null bitmap and a ~2e9-iteration padding loop. `stack_take` would also build an 8 GB `Vec<u32>` of indices. A modest `n` (e.g. 100000) times an 8192-row batch is large too. Constant folding of an all-scalar call can reach this at plan time. Spark streams these rows lazily, and there is no memory accounting here. Could we cap the output size, or at least fail cleanly? 2. **Type unification depends on argument order (~L356).** An integer literal only unifies with INT when the INT value comes first. `stack(2, 0, a)` with `a INT` fails with an Int64 vs Int32 mismatch, while `stack(2, a, 0)` works. Spark's integer literals are INT, so common unpivots like `stack(2, 'x', 1, 'y', int_col)` break depending on order. The slt test for this case is commented out as a known gap. Preferring the non-literal type for a column, or treating integer literals that fit in INT as Int32, would fix it. 3. **`n` must be a bare literal (~L266).** `stack(1 + 1, ...)`, `stack(2::INT, ...)` and `stack(CAST(2 AS INT), ...)` fail at planning. The error says the expression is not foldable when it is. **Smaller items** 4. **`same_field` compares nullability (~L376).** Spark's `sameType` ignores nullability, so `stack(2, arr_a, arr_b)` with `List<Int64 not null>` and `List<Int64 nullable>` is rejected. The same applies to struct fields and map values. `FixedSizeList` vs `List` also never matches. 5. **Strict execution-time cast (~L443).** `invoke_with_args` uses `cast_to` with `safe: false`. A caller that supplies its own `return_field` (e.g. Comet) with an Int32 column and Int64 data above `i32::MAX` fails the whole batch with a generic Arrow cast error. 6. **Broadcast is inferred from length (~L519).** A source counts as per-row when `source.len() > 1`, and the check is repeated in `stack_take`, the strided loops and the all-len-1 check that gates `tile`. If an array's length differs from `number_rows`, `zip` silently truncates and `stack_take` indexing can go out of bounds. Tracking scalar-ness once, e.g. with a `broadcast` flag computed in `stack_columns`, would remove the implicit invariant. 7. **Three kernel paths (~L592).** `tile` (doubling + concat for all-scalar Utf8/Binary columns) sits beside `stack_take` and `stack_primitive` for one benchmark shape. It has one slt case, and the 256-copy block boundary is only covered indirectly. A single path for scalar-only columns would be easier to maintain. I'd treat 1-3 as the ones to address before merging, especially 1 and 3. -- 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]
