cudf-polars: preserve zero-column row counts - #23234
Conversation
f51d8e7 to
4589c64
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe DataFrame container now stores zero-column row counts through ChangesZero-column row-count handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mroeschke
left a comment
There was a problem hiding this comment.
Just a suggestion for a cleanup (can be a follow up) otherwise LGTM
| ), | ||
| stream=stream, | ||
| num_rows=num_rows, | ||
| num_rows=table.num_rows() if num_rows is None else num_rows, |
There was a problem hiding this comment.
suggestion: Now that pylibcudf.Table accepts a num_rows argument, would be good to see if the we can remove num_rows from this function and have the caller always pass in a table with the intended row count
…om:madsbk/cudf into cudf-polars-zero-column-with-multiple-rows
…-column-with-multiple-rows
wence-
left a comment
There was a problem hiding this comment.
One question about the comment in with_columns
| # When the result has columns, the row count is derived from them (and the | ||
| # non-broadcast path deliberately allows mismatched lengths, so we must not | ||
| # force a num_rows here). | ||
| return type(self)( | ||
| merged.values(), | ||
| stream=stream, | ||
| num_rows=self.num_rows if not merged else None, | ||
| ) |
There was a problem hiding this comment.
I think it is invalid to pass new columns that have a different length from the existing table. So what is this comment saying?
There was a problem hiding this comment.
Not always, updated the comment:
# Only pass num_rows for a zero-column result. For results with columns, it
# must remain None because HStack(should_broadcast=False) intentionally
# produces mismatched column lengths that its Select parent reconciles later.
return type(self)(
merged.values(),
stream=stream,
num_rows=self.num_rows if not merged else None,
)…-column-with-multiple-rows
|
/merge |
…ction DataFrame.from_table lost its num_rows parameter in NVIDIA#23234 (row counts are now inferred from the pylibcudf table, which carries them even for zero-column tables), but the duplicated-output path added in NVIDIA#23114 still passed num_rows=0, breaking mypy on every PR and raising TypeError at runtime on that path. empty_like already produces a 0-row table (including for zero-column inputs), so the argument was redundant.
…utput path (#23303) `DataFrame.from_table` lost its `num_rows` parameter in #23234 (row counts are now inferred from the pylibcudf table, which carries them even for zero-column tables), but the duplicated-output path added in #23114 still passes `num_rows=0`. The two PRs merged around the same time, so this surfaced only after both landed: mypy now fails on every PR's `check-style` job (`engine/core.py:840: Unexpected keyword argument "num_rows"`), and the path would raise `TypeError` at runtime. `plc.copying.empty_like` already produces a 0-row table (including for zero-column inputs, verified), so dropping the argument preserves the intended "freshly-allocated empty same-schema frame" semantics exactly. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Matthew Roeschke (https://github.com/mroeschke) - Lawrence Mitchell (https://github.com/wence-) URL: #23303
…utput path (NVIDIA#23303) `DataFrame.from_table` lost its `num_rows` parameter in NVIDIA#23234 (row counts are now inferred from the pylibcudf table, which carries them even for zero-column tables), but the duplicated-output path added in NVIDIA#23114 still passes `num_rows=0`. The two PRs merged around the same time, so this surfaced only after both landed: mypy now fails on every PR's `check-style` job (`engine/core.py:840: Unexpected keyword argument "num_rows"`), and the path would raise `TypeError` at runtime. `plc.copying.empty_like` already produces a 0-row table (including for zero-column inputs, verified), so dropping the argument preserves the intended "freshly-allocated empty same-schema frame" semantics exactly. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Matthew Roeschke (https://github.com/mroeschke) - Lawrence Mitchell (https://github.com/wence-) URL: NVIDIA#23303
Description
Follow-up to #22765. Propagate the row count of a zero-column table through the cudf-polars
DataFramecontainer andProjectionnode, so a frame with no columns andNrows preserves its shape as(N, 0)instead of collapsing to(0, 0).