perf(arrow-cast): gate Dictionary -> View fast path on cardinality#10436
Open
Abhisheklearn12 wants to merge 3 commits into
Open
perf(arrow-cast): gate Dictionary -> View fast path on cardinality#10436Abhisheklearn12 wants to merge 3 commits into
Abhisheklearn12 wants to merge 3 commits into
Conversation
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.
Which issue does this PR close?
Supersedes #9768, which I closed it implemented these fast paths ungated, and benchmarking showed that made things significantly slower. Details below.
Rationale for this change
dictionary_castcan build a view array from a dictionary two ways:unpack_dictionarybuilds one view per dictionary value, then gathers withtakeview_from_dict_valuesbuilds one view per row directly against the values bufferThe comment above (b) says (a) "incurs unnecessary data copy of the value buffer". That
isn't the case.
impl From<&GenericByteArray> for GenericByteViewArrayreuses the buffer(it only falls back to copying when the values exceed the
u32offset a view can hold),and
take_byte_viewpassesdata_buffers().to_vec()through untouched. Neither stepcopies value data, so there was no copy to remove.
What the two actually trade is a bandwidth-bound
u128gather intakeagainst aper-row
make_viewthat reads each row's payload out of the values buffer. The gatherwins once rows reach roughly 0.6x the dictionary size so (b) is a large pessimisation in
the common case, and only pays off when the dictionary is substantially larger than the
array is long, where (a) spends most of its time building views no row references.
This means the existing
Utf8 -> Utf8ViewandBinary -> BinaryViewfast paths on mainare currently 5-8x slower than
unpack_dictionaryat typical batch shapes.What changes are included in this PR?
keys.len() < values.len() / 2; everything else falls throughto
unpack_dictionary. The measured crossover is nearrows ≈ 0.6 * values; the gateswitches short of it, at
rows = values / 2, where the direct path is still ahead by15-30%. That margin covers the crossover moving with cache size or microarchitecture.
Being wrong in that direction only forgoes a win; being wrong the other way is a
regression on the common case.
LargeUtf8/LargeBinary->Utf8View/BinaryView, and theUtf8<->Binarycrosscasts with the offset-fit check and UTF-8 validation the issue calls out.
view_from_dict_valuesdropping null dictionary values: they became empty stringsrather than nulls, where
unpack_dictionaryandimpl From<&GenericByteArray>bothproduce nulls.
from undefined behaviour into an error. The gate confines this loop to short row counts,
so it is within noise when the dictionary holds no nulls. When the dictionary does hold
nulls, the null check costs 25-40% on the direct path an unavoidable random bitmap
lookup per row, and the price of emitting nulls rather than the empty strings the
previous code produced.
Benchmarks
Existing fast paths on main:
Utf8->Utf8ViewUtf8->Utf8ViewBinary->BinaryViewBinary->BinaryViewNew arms, sparse shapes (dictionary larger than the array):
LargeUtf8->Utf8ViewLargeUtf8->BinaryViewLargeBinary->BinaryViewUtf8->BinaryViewBinary->Utf8ViewLargeBinary->Utf8View(
Binary/LargeBinary -> Utf8Viewgain less because UTF-8 validation of the dictionaryvalues dominates.)
Dense shapes on the new arms are unchanged, -2.9% to +1.2% the gate routes them to
unpack_dictionary. Worst cell across the whole matrix is +5.0%, on a shape where bothversions take the direct path and the delta is the null and bounds checks above.
Method
Both implementations were compiled into a single binary and timed in alternating rounds so
thermal drift cancels out of the ratio. Cells whose code is identical in both versions come
out at -2.9% to +1.2%, which bounds the method's noise; a two-run criterion comparison of
those same cells reported up to +47% drift, which is why it wasn't used. Outputs are
asserted equal per cell before timing. Measured on an i7-11700F; a second independent run
reproduced every headline result within 3.2 points and produced the identical set of cells
above 50%; the largest shift on any cell was 8.3 points, on a cell that is flat in both runs.
Are these changes tested?
Yes. Every arm is exercised through both implementations (row counts either side of the
gate), with results asserted equal to the
unpack_dictionaryreference in each case. Nulldictionary values, null keys, and invalid UTF-8 under both
safeand strictCastOptionsare covered by dedicated tests.
Are there any user-facing changes?
Casting
Dictionary<_, Utf8> -> Utf8ViewandDictionary<_, Binary> -> BinaryViewbecomessubstantially faster at typical batch shapes 5-8x on the shapes benchmarked above.
Dictionary<_, LargeUtf8/LargeBinary> -> Utf8View/BinaryViewand theUtf8<->Binarycross casts gain a fast path when the dictionary holds more than twice as many values as the
array has rows. Outside that they behave as before, going through
unpack_dictionary.Two behaviour changes, called out explicitly:
InvalidArgumentErrorinstead of beingundefined behaviour. Only reachable for a dictionary built without validation.
Happy to split either into its own PR if you'd prefer this one stay purely about the fast path.