[opt](hive) vectorize single-char hive text split and cache KMP prefix table - #66164
Closed
Baymine wants to merge 1 commit into
Closed
[opt](hive) vectorize single-char hive text split and cache KMP prefix table#66164Baymine wants to merge 1 commit into
Baymine wants to merge 1 commit into
Conversation
…x table Issue Number: close apache#66163 Problem Summary: Hive text field splitting has two avoidable costs on the hot scan path: 1. The single-char separator path scanned the line byte-by-byte even when no escape char is configured (the common case). Replace that loop with a memchr-based scan when escape_char == 0, which lets the C library use a vectorized separator search. The escape path is unchanged and still uses is_hive_text_separator_escaped. 2. The multi-char (MultiDelimitSerDe) path rebuilt the KMP prefix table on every line. Precompute it once in the constructor and reuse it per call. Both changes are behavior-preserving; split results are identical. Release note: None
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
Closing this: a more rigorous benchmark (interleaved runs, pinned core) shows the memchr fast path regresses ~10-14% for sub-6-byte fields (break-even at ~6 bytes, only a clear win at >=20 bytes), which confirms the existing 'for+if beats memchr for short fields' note. Will revisit with a width-gated hybrid if pursued. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Issue Number: close #66163
Problem Summary
HiveTextFieldSplittersplits every Hive text line on the hot scan path. This PR removes two avoidable costs, both behavior-preserving (split results are identical):Single-char separator, no escape char (common case). The
escape_char == 0branch of_split_field_single_charscanned the line byte-by-byte. It now usesmemchrto locate each separator, letting glibc use its vectorized search. The escape path is unchanged and still usesis_hive_text_separator_escaped.Multi-char separator (MultiDelimitSerDe).
_split_field_multi_charrebuilt the KMP prefix table from the separator on every line. It is now computed once in the constructor (_kmp_next) and reused per call. This is a strict win (no downside).Benchmark (memchr vs for+if, single-char path)
There is an existing note in
new_plain_text_line_reader.cppthatfor + ifbeatsmemchr"under normal short fields case". A micro-benchmark of the split loop (plain-O2, 4M iters/case) shows the tradeoff precisely:memchrwins for field widths >= ~6 bytes (the normal case for real Hive columns: strings, numbers, timestamps, paths) and is dramatically faster for wide values. The only regression is ~5% for pathologically tiny 1-3 byte fields, which matches the existing note. If reviewers prefer, the fast path can be gated behind a small line-length threshold to keepfor + iffor tiny lines; happy to add that.Release note
None
Check List (For committer)
Test
Behavior changed:
Does this need documentation?
Check List (For author)
Unit Test:
be/test/format/text/hive_text_field_splitter_test.cppgains 5 cases covering the memchr fast-path boundaries (Hive\x01default separator, long fields spanning SIMD chunks, consecutive/leading/trailing separators, multi-byte UTF-8), the escape vs no-escape divergence, the cached KMP table reused across rows, and the multi-char escape boundary conditions.HiveTextFieldSplitterTest10/10 pass locally.Manual test: micro-benchmark of the split loop (results above).
I have unit-tested and manually tested this change.