perf(sparksql): eliminate data copy in unscaled_value#694
Closed
zhli1142015 wants to merge 1 commit into
Closed
Conversation
### What problem does this PR solve? `unscaled_value` returns the raw `int64` of a short decimal as a `BIGINT`. The input and output share the same physical type (`int64_t`) and differ only in logical type, yet the implementation copied every row into a freshly allocated `FlatVector`. That per-row copy plus the extra allocation is pure overhead on a zero-transformation function. Issue Number: N/A (port of facebookincubator/velox#13023) ### Type of Change - [ ] 🐛 Bug fix (non-breaking change which fixes an issue) - [ ] ✨ New feature (non-breaking change which adds functionality) - [x] 🚀 Performance improvement (optimization) - [ ]⚠️ Breaking change (fix or feature that would cause existing functionality to change) - [ ] 🔨 Refactoring (no logic changes) - [ ] 🔧 Build/CI or Infrastructure changes - [ ] 📝 Documentation only ### Description Reuse the input's physical representation instead of copying it: - **Constant encoding**: pass the scalar value through a new `ConstantVector<int64_t>` typed as `BIGINT`. - **Flat encoding**: build the `BIGINT` result on top of the input's `values()` buffer (shared, no copy), then hand off via `EvalCtx::moveOrCopyResult`, which copies lazily only when the result buffer must be preserved. Null handling is unchanged: the function keeps default-null behavior, so the expression framework applies input nulls to the output (the flat result is built with a null buffer of `nullptr`). ### Performance Impact - [ ] **No Impact**: This change does not affect the critical path (e.g., build system, doc, error handling). - [x] **Positive Impact**: Eliminates a per-row copy and a full-vector allocation; the flat path now shares the input buffer (zero-copy) and only copies lazily when the result must be preserved. - [ ] **Negative Impact**: Explained below (e.g., trade-off for correctness). ### Release Note Release Note: - Optimized `unscaled_value` by eliminating a per-row data copy and reusing the input's physical buffer. ### Checklist (For Author) - [x] I have added/updated unit tests (ctest). - [ ] I have verified the code with local build (Release/Debug). - [ ] I have run clang-format / linters. - [ ] (Optional) I have run Sanitizers (ASAN/TSAN) locally for complex C++ changes. - [ ] No need to test or manual test. ### Breaking Changes - [x] No - [ ] Yes (Description: ...) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
What problem does this PR solve?
unscaled_valuereturns the rawint64of a short decimal as aBIGINT. The input and output share the same physical type (int64_t) and differ only in logical type, yet the implementation copied every row into a freshly allocatedFlatVector. That per-row copy plus the extra allocation is pure overhead on a zero-transformation function.Issue Number: N/A
Type of Change
Description
Reuse the input's physical representation instead of copying it:
ConstantVector<int64_t>typed asBIGINT.BIGINTresult on top of the input'svalues()buffer (shared, no copy), then hand off viaEvalCtx::moveOrCopyResult, which copies lazily only when the result buffer must be preserved.Null handling is unchanged: the function keeps default-null behavior, so the expression framework applies input nulls to the output (the flat result is built with a null buffer of
nullptr).Performance Impact
Release Note
Release Note:
unscaled_valueby eliminating a per-row data copy and reusing the input's physical buffer.Checklist (For Author)
Breaking Changes