From 645725eb29e51d82eb08e384438781aad8677d4d Mon Sep 17 00:00:00 2001 From: zhli1142015 Date: Wed, 1 Jul 2026 22:14:09 +0800 Subject: [PATCH] perf(sparksql): eliminate data copy in unscaled_value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### 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` 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> --- .../sparksql/UnscaledValueFunction.cpp | 26 ++++++++++++------- .../tests/UnscaledValueFunctionTest.cpp | 20 +++++++------- 2 files changed, 27 insertions(+), 19 deletions(-) diff --git a/bolt/functions/sparksql/UnscaledValueFunction.cpp b/bolt/functions/sparksql/UnscaledValueFunction.cpp index 69fbc7cf5..d3b8a7e7c 100644 --- a/bolt/functions/sparksql/UnscaledValueFunction.cpp +++ b/bolt/functions/sparksql/UnscaledValueFunction.cpp @@ -47,15 +47,23 @@ class UnscaledValueFunction final : public exec::VectorFunction { args[0]->type()->isShortDecimal(), "Expect short decimal type, but got: {}", args[0]->type()); - exec::DecodedArgs decodedArgs(rows, args, context); - auto decimalVector = decodedArgs.at(0); - context.ensureWritable(rows, BIGINT(), result); - result->clearNulls(rows); - auto flatResult = - result->asUnchecked>()->mutableRawValues(); - rows.applyToSelected([&](auto row) { - flatResult[row] = decimalVector->valueAt(row); - }); + VectorPtr localResult; + const auto& arg = args[0]; + if (arg->isConstantEncoding()) { + auto value = arg->as>()->valueAt(0); + localResult = std::make_shared>( + context.pool(), rows.end(), false, BIGINT(), std::move(value)); + } else { + auto flatInput = arg->asFlatVector(); + localResult = std::make_shared>( + context.pool(), + BIGINT(), + nullptr, + rows.end(), + flatInput->values(), + std::vector()); + } + context.moveOrCopyResult(localResult, rows, result); } }; } // namespace diff --git a/bolt/functions/sparksql/tests/UnscaledValueFunctionTest.cpp b/bolt/functions/sparksql/tests/UnscaledValueFunctionTest.cpp index 5aa19cd64..311a9664d 100644 --- a/bolt/functions/sparksql/tests/UnscaledValueFunctionTest.cpp +++ b/bolt/functions/sparksql/tests/UnscaledValueFunctionTest.cpp @@ -37,18 +37,18 @@ namespace { class UnscaledValueFunctionTest : public SparkFunctionBaseTest {}; TEST_F(UnscaledValueFunctionTest, unscaledValue) { - auto testUnscaledValue = [&](const std::vector& unscaledValue, - const TypePtr& decimalType) { - auto input = makeFlatVector(unscaledValue, decimalType); - auto expected = makeFlatVector(unscaledValue); - auto result = evaluate("unscaled_value(c0)", makeRowVector({input})); - assertEqualVectors(expected, result); - }; + auto input = + makeFlatVector({1000, 2000, -3000, -4000}, DECIMAL(18, 3)); + auto expected = makeFlatVector({1000, 2000, -3000, -4000}); + auto invalidInput = makeFlatVector({0, 0, 0, 0}, DECIMAL(20, 3)); - testUnscaledValue({1000, 2000, -3000, -4000}, DECIMAL(18, 3)); + testEncodings( + makeTypedExpr("unscaled_value(c0)", ROW({"c0"}, {input->type()})), + {input}, + expected); - BOLT_ASSERT_THROW( - testUnscaledValue({1000, 2000, -3000, -4000}, DECIMAL(20, 3)), + BOLT_ASSERT_USER_THROW( + evaluate("unscaled_value(c0)", makeRowVector({invalidInput})), "Expect short decimal type, but got: DECIMAL(20, 3)"); }