[VL] Offload 2-arg decimal ceiling/floor to Velox native execution - #12777
Open
minni31 wants to merge 1 commit into
Open
[VL] Offload 2-arg decimal ceiling/floor to Velox native execution#12777minni31 wants to merge 1 commit into
minni31 wants to merge 1 commit into
Conversation
Spark's `ceiling(x, scale)` / `floor(x, scale)` on decimal inputs produce `RoundCeil(decimal, scale)` / `RoundFloor(decimal, scale)`. These were mapped to the substrait `ceil` / `floor` function names but had no native execution path, so the 2-arg decimal form fell back to vanilla Spark. This wires them to the Velox `decimal_ceil` / `decimal_floor` special forms: - SubstraitParser: `mapToVeloxFunction` gains a `numArgs` parameter and remaps 2-arg `ceil` / `floor` on decimals to `decimal_ceil` / `decimal_floor`. Unary `ceil(decimal)` / `floor(decimal)` keep their existing name. - DecimalCeilFloorTransformer: new transformer mirroring DecimalRoundTransformer; recomputes the output DecimalType from the input type and constant-folded scale (matching Spark's RoundBase.dataType) and emits the scale as a literal. - ExpressionConverter: route decimal RoundCeil / RoundFloor through the new transformer. Addresses the RoundCeil / RoundFloor items in apache#10134.
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enables native (Velox) execution for Spark’s 2-argument decimal ceiling(x, scale) / floor(x, scale) by routing RoundCeil / RoundFloor through Substrait ceil/floor and remapping those calls (when decimal + arity=2) to Velox’s decimal_ceil / decimal_floor special forms.
Changes:
- Add a new
DecimalCeilFloorTransformerto constant-fold the scale, recompute the outputDecimalType, and emit the scale as a literal argument. - Update
ExpressionConverterto use the new transformer for decimalRoundCeil/RoundFloor. - Update Velox Substrait parsing to remap decimal
ceil/floortodecimal_ceil/decimal_floorwhen the call arity is 2, and add coverage inMathFunctionsValidateSuite.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala | Routes decimal RoundCeil / RoundFloor through the new decimal ceil/floor transformer. |
| gluten-substrait/src/main/scala/org/apache/gluten/expression/DecimalCeilFloorTransformer.scala | New transformer that folds scale, recomputes output decimal type, and emits a literal scale for Substrait. |
| cpp/velox/substrait/SubstraitParser.h | Extends function name remapping API to accept an optional argument-count discriminator. |
| cpp/velox/substrait/SubstraitParser.cc | Uses arity + decimal detection to remap 2-arg ceil/floor to decimal_ceil/decimal_floor. |
| backends-velox/src/test/scala/org/apache/gluten/functions/MathFunctionsValidateSuite.scala | Adds offload validation tests for 2-arg decimal ceiling/floor with positive/negative scales. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+42
to
+49
| private val toScale: Int = { | ||
| val evaluated = scaleExpr.eval(EmptyRow) | ||
| if (evaluated == null) { | ||
| throw new GlutenNotSupportException( | ||
| s"Scale expression evaluated to null for ${original.nodeName}. Falling back to Spark.") | ||
| } | ||
| evaluated.asInstanceOf[Int] | ||
| } |
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 changes are proposed in this pull request?
Spark's
ceiling(x, scale)/floor(x, scale)on decimal inputs produceRoundCeil(decimal, scale)/RoundFloor(decimal, scale). These expressions were mapped to the substraitceil/floorfunction names viaExpressionMappings, but there was no native execution path for the 2-argument decimal form, so it silently fell back to vanilla Spark.This PR wires the 2-arg decimal forms to the Velox
decimal_ceil/decimal_floorspecial forms:SubstraitParser::mapToVeloxFunctiongains anumArgsparameter (defaulting to0) and remaps 2-argceil/flooron decimals todecimal_ceil/decimal_floor. Unaryceil(decimal)/floor(decimal)keep their existing name and native path, so there is no change to the 1-arg behavior.DecimalCeilFloorTransformer(new) mirrors the existingDecimalRoundTransformer: it recomputes the outputDecimalTypefrom the input decimal type and the constant-folded scale (matching Spark'sRoundBase.dataType), and emits the scale as a literal argument.ExpressionConverterroutes decimalRoundCeil/RoundFloorthrough the new transformer, consistent with how decimalRoundis handled.The ClickHouse path is unchanged: the substrait function names (
ceil/floor) are preserved, so only the Velox backend remaps them on the C++ side.This addresses the
RoundCeil/RoundFlooritems tracked in #10134.How was this patch tested?
Added a native offload test in
MathFunctionsValidateSuitecovering 2-argceiling/flooron decimal inputs with both positive and negative scales. It usesrunQueryAndCompare(validates results against vanilla Spark) and asserts the projection is offloaded viacheckGlutenPlan[ProjectExecTransformer].Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot