apache / apache/datafusion

Reduce copying in `CoalesceBatchesExec` for StringViews

オープン
#11,628 コメント 3 件 リアクション 0 件 担当者 0 名 GitHub で見る
enhancement
主要言語
Rust
スター
9.3k
フォーク
2.4k
平均マージ
3日 11時間
マージ済み PR(30日)
360

説明

### Is your feature request related to a problem or challenge?

In pictures, what https://github.com/apache/datafusion/pull/11587 does is like
this (to ensure lots of unreachable "garbage" does not accumulate in the output batch)

```
┌────────────────────┐
│ RecordBatch │ ┌────────────────────┐
│ num_rows = 23 │ │ RecordBatch │
└────────────────────┘ │ num_rows = 23 │ ┌────────────────────┐
└────────────────────┘ │ │
┌────────────────────┐ Coalesce │ │
│ │ StringView::gc ┌────────────────────┐ Batches │ │
│ RecordBatch │ │ RecordBatch │ │ │
│ num_rows = 50 │ │ num_rows = 50 │ ─ ─ ─ ─ ─▶ │ │
│ │ ─ ─ ─ ─ ─▶ │ │ │ RecordBatch │
│ │ └────────────────────┘ │ num_rows = 106 │
└────────────────────┘ │ │
│ │
┌────────────────────┐ ┌────────────────────┐ │ │
│ │ │ RecordBatch │ │ │
│ RecordBatch │ │ num_rows = 33 │ │ │
│ num_rows = 33 │ │ │ └────────────────────┘
│ │ └────────────────────┘
└────────────────────┘
```

However, as @2010YOUY01 pointed out in https://github.com/apache/datafusion/pull/11587/files#r1686678665

> So here inside gc string buffer will be copied once, (below) in
> concat_batches() string buffer will be copied again, it seems possible to copy
> only once by changing the internal implementation of concat_batches()

This implementation will effectively copy the data twice -- once for the call to
`gc` and once for the call coalsece batches.

Due to the nature of `StringView` the actual strings vaules are only copied once, but the `u128` view value will be copied twice

### Describe the solution you'd like

Somehow structure the code to avoid copying the views again. Like this

```
┌────────────────────┐
│ RecordBatch │
│ num_rows = 23 │ ┌────────────────────┐
└────────────────────┘ │ │
StringView::gc │ │
┌────────────────────┐ and Coalesce │ │
│ │ Batches in same │ │
│ RecordBatch │ operation │ │
│ num_rows = 50 │ │ RecordBatch │
│ │ ─ ─ ─ ─ ─▶ │ num_rows = 106 │
│ │ │ │
└────────────────────┘ │ │
│ │
┌────────────────────┐ │ │
│ │ │ │
│ RecordBatch │ └────────────────────┘
│ num_rows = 33 │
│ │
└────────────────────┘
```

### Describe alternatives you've considered

https://github.com/apache/datafusion/pull/11587/files#r1687099239

> I think given how concat is implemented for `StringView` it will only copy the fixed parts (not the actual string data)
>
> Perhaps what we could do is implement a wrapper around arrow::concat_batches that has the datafusion specific GC trigger for sparse arrays, and falls back to concat for other types: https://docs.rs/arrow-select/52.1.0/src/arrow_select/concat.rs.html#150
>
> ```rust
> /// wrapper around [`arrow::compute::concat`] that
> pub fn concat(arrays: &[&dyn Array]) -> Result {
> // loop over columns here and handle StringView specially,
> // or fallback to concat
> }
> ```

### Additional context

https://github.com/apache/datafusion/issues/7957 is another related idea for avoding copies

コントリビューションガイド

コントリビューションガイドを開く

調査の方向性

まず CoalesceBatchesExec と concat_batches の実装を読み、次に PR 11587 と StringView::gc に関する関連ディスカッションを確認します。現在 StringView 配列がどのように扱われているか、またガベージコレクションとバッチ連結で 1 つの操作を共有できるかを判断してください。既存のバッチ動作を維持しながら、StringView の u128 値の 2 回目のコピーを回避できれば完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
rust
領域
backend, performance
issue の種類
機能追加
難易度
4/5
見積もり時間
3〜5日
活発さ
停滞
明瞭さ
説明が足りない
初心者へのやさしさ
35/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。