bowenliang123 opened a new pull request, #7609:
URL: https://github.com/apache/kyuubi/pull/7609
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug, and what versions
are affected.
-->
`TRowSet` generation is on the hot path of every result fetch, and the
current implementation allocates several intermediate structures that are pure
overhead:
- `TColumnGenerator.getColumnToList` iterates with a `foreach` closure that
captures the mutable row index and the null bitmap (boxed into heap `IntRef`s
per call), re-checks `convertFunc == null` for every row, and starts the null
`BitSet` at the default 64-bit capacity, paying repeated growth copies for wide
result sets.
- `asFloatTColumn` and `asByteArrayTColumn` run a second pass over the
produced column (`asScala.map(...).asJava`), materializing an entire
intermediate boxed list (`JFloat` / `Array[Byte]`) plus JavaConverters wrappers
before the real `TDoubleColumn`/`TBinaryColumn` list exists.
- `TRowSetGenerator.toColumnBasedSet` pre-sizes the `rows` list with
`rowSize` capacity even though columnar layout never stores any row in it — the
array is pure retained garbage.
The changes:
- `getColumnToList`: `while` loop with an explicit iterator (index/bitmap
stay in registers), the `convertFunc == null` branch is hoisted out of the
loop, and the `BitSet` is pre-sized to `rowSize`.
- `asFloatTColumn` / `asByteArrayTColumn`: single-pass via `convertFunc`,
producing `JList[JDouble]` / `JList[ByteBuffer]` directly. The float -> string
-> double conversion (which preserves float display semantics) is kept
unchanged.
- `toColumnBasedSet`: the always-empty `rows` list is created with zero
capacity.
No wire-protocol or public API change; behavior (values and null bitmaps) is
byte-for-byte identical.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some
test cases that check the changes thoroughly including negative and positive
cases if possible.
If it was tested in a way different from regular unit tests, please clarify
how you tested step by step, ideally copy and paste-able, so that other
reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why
it was difficult to add.
-->
No new unit tests: the patch is a behavior-preserving refactor of internal
implementation, and existing engine `RowSetSuite`s (Spark/Flink/Trino) already
cover these paths end-to-end in CI.
Correctness was verified with a standalone harness that runs the old and new
implementations side by side over 100k-row columns and asserts the produced
value lists and null bitmaps are equal, covering string/integer/float/binary
columns, with and without `convertFunc`, and with 0%/10% nulls.
Measured gains (JDK 17, Scala 2.12, 100k rows per column, 50 interleaved
samples per implementation, median; allocation measured via
`ThreadMXBean.getCurrentThreadAllocatedBytes`):
| Scenario | Time (old -> new) | Allocation (old -> new) |
|---|---|---|
| String column, 10% nulls, no convertFunc | 0.41 -> 0.41 ms (~1.0x) | 435
-> 415 KB/op (-20 KB, BitSet pre-sizing) |
| Integer column (boxed), 10% nulls, no convertFunc | 0.38 -> 0.38 ms
(~1.0x) | 435 -> 415 KB/op (-20 KB) |
| Float column (`asFloatTColumn`), 10% nulls | 8.39 -> 7.51 ms (**1.12x**) |
14.6 -> 12.6 MB/op (**1.16x**, intermediate `JFloat` list eliminated) |
| Binary column (`asByteArrayTColumn`), 10% nulls | 1.41 -> 0.90 ms
(**1.57x**) | 6.9 -> 5.3 MB/op (**1.30x**) |
| `toColumnBasedSet` rows list (per result set) | — | 391 KB saved per 100k
rows, 3.9 MB per 1M rows (retained, not garbage) |
Also verified: `build/mvn -Pfast clean install -pl kyuubi-common -am
-DskipTests`, `build/mvn spotless:check -pl kyuubi-common`, and `dev/reformat`.
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has assisted in authoring or reviewing this patch,
please include
an 'Assisted-by: ' trailer that identifies the agent and model.
For example: 'Assisted-by: Claude:claude-opus-4-7' or 'Assisted-by: Claude
Opus 4.7'.
If no, write 'No'.
Please refer to the [ASF Generative Tooling
Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
Assisted-by: Kimi Code CLI (Moonshot AI)
--
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]
