[VL] Support format_number function - #12754
Conversation
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
This PR wires Spark SQL format_number(number, decimalPlaces) through Gluten’s Substrait expression mapping so Velox can execute it natively where supported, and adds coverage plus documentation/restriction metadata.
Changes:
- Add
format_numbertoExpressionNamesand registerFormatNumberinExpressionMappings. - Document Velox support as partial (numeric primitives only; DecimalType excluded) and add a corresponding restriction entry for doc generation.
- Add Velox backend validation tests for
format_number, including expected Spark-side execution for DecimalType input.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shims/common/src/main/scala/org/apache/gluten/expression/ExpressionNames.scala | Adds the FORMAT_NUMBER function name constant used across shims/mappings. |
| gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionMappings.scala | Maps Spark’s FormatNumber expression to Substrait function name format_number. |
| docs/velox-backend-scalar-function-support.md | Updates function support table to reflect partial support + DecimalType restriction note. |
| backends-velox/src/test/scala/org/apache/gluten/functions/ScalarFunctionsValidateSuite.scala | Adds UTs covering offload for supported numeric types and non-offload for DecimalType input. |
| backends-velox/src/main/scala/org/apache/gluten/expression/ExpressionRestrictions.scala | Adds a FormatNumberRestrictions entry for doc/restriction reporting. |
| backends-clickhouse/src/main/scala/org/apache/gluten/utils/CHExpressionUtil.scala | Blacklists format_number for ClickHouse backend to avoid incorrect pushdown. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
backends-velox/src/test/scala/org/apache/gluten/functions/ScalarFunctionsValidateSuite.scala:768
- The added test comments say this block exercises HALF_EVEN rounding and thousands separators, but the current inputs from TPCH columns are not deterministic for those edge cases (e.g., they may not hit tie-to-even cases, and values may not exceed 999 to guarantee separators). Adding deterministic cases (using
datatab.double_field1which already includes 1.025/1.035/1.045, and a scaled integer) will make the intent of the test reliably true.
// Floating-point input, exercising HALF_EVEN rounding and thousands separators.
runQueryAndCompare(
"SELECT format_number(cast(l_quantity as double), 1) FROM lineitem limit 50") {
checkGlutenPlan[ProjectExecTransformer]
}
20b7e3d to
ba2e2b4
Compare
|
Run Gluten Clickhouse CI on x86 |
ba2e2b4 to
9669af7
Compare
|
Run Gluten Clickhouse CI on x86 |
9669af7 to
44e0caf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
backends-clickhouse/src/main/scala/org/apache/gluten/utils/CHExpressionUtil.scala:218
- This PR is scoped/tagged as [VL] and the description focuses on Velox wiring, but it also changes ClickHouse validation behavior. If the intention is only Velox enablement, consider dropping this CH change (or move it to a separate PR / explicitly document why CH needs to treat
format_numberas validated here).
DIV -> DefaultValidator(),
REGEXP_INSTR -> DefaultValidator(),
DAY_NAME -> DefaultValidator(),
MONTH_NAME -> DefaultValidator(),
FORMAT_NUMBER -> DefaultValidator()
docs/velox-backend-scalar-function-support.md:373
- The restriction text here duplicates the canonical restriction message added in
ExpressionRestrictions.scala(which is also referenced as input togen-function-support-docs.py). To avoid drift, prefer generating/updating this doc entry from the restriction source, or reusing exactly the same wording from a single source of truth.
| format_number | FormatNumber | PS | format_number only supports tinyint, smallint, integer, bigint, float and double input; DecimalType input is not supported in Velox |
backends-velox/src/main/scala/org/apache/gluten/expression/ExpressionRestrictions.scala:111
- Constant name
NOT_SUPPORT_DECIMAL_INPUTis grammatically inconsistent and reads like a verb phrase. Consider renaming toNOT_SUPPORTED_DECIMAL_INPUT(or similar) to match common naming conventions and improve readability.
val NOT_SUPPORT_DECIMAL_INPUT: String =
s"${ExpressionNames.FORMAT_NUMBER} only supports tinyint, smallint, integer, bigint, " +
s"float and double input; DecimalType input is not supported in Velox"
backends-velox/src/main/scala/org/apache/gluten/expression/ExpressionRestrictions.scala:111
- Constant name
NOT_SUPPORT_DECIMAL_INPUTis grammatically inconsistent and reads like a verb phrase. Consider renaming toNOT_SUPPORTED_DECIMAL_INPUT(or similar) to match common naming conventions and improve readability.
override val restrictionMessages: Array[String] = Array(NOT_SUPPORT_DECIMAL_INPUT)
|
Run Gluten Clickhouse CI on x86 |
zml1206
left a comment
There was a problem hiding this comment.
Spark officially supports two forms of format_number: an integer number of decimal places and a user-specified string format. Velox currently implements only the integer form.
However, the restriction metadata and generated support documentation mention only unsupported DecimalType input, so the documented support scope is incomplete. Could we also document the unsupported string-format argument and add a fallback test using the example from Spark’s documentation, such as:
format_number(12332.123456, '##################.###')
The test should verify that this form falls back to ProjectExec.
44e0caf to
6173798
Compare
|
Run Gluten Clickhouse CI on x86 |
6173798 to
dc44a93
Compare
|
Run Gluten Clickhouse CI on x86 |
philo-he
left a comment
There was a problem hiding this comment.
LGTM. One trivial comment. Thanks.
| | endswith | EndsWithExpressionBuilder | PS | BinaryType unsupported | | ||
| | find_in_set | FindInSet | S | | | ||
| | format_number | FormatNumber | | | | ||
| | format_number | FormatNumber | PS | format_number only supports tinyint, smallint, integer, bigint, float and double input; DecimalType input is not supported in Velox<br>format_number with a string format argument (e.g. '#,###.##') is not supported in Velox; only an integer number of decimal places is supported | |
There was a problem hiding this comment.
Could you help update the PS function number?
dc44a93 to
48c36d8
Compare
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
backends-velox/src/test/scala/org/apache/gluten/functions/ScalarFunctionsValidateSuite.scala:779
- The query
format_number(..., '#,###.##')uses the string-pattern overload, which appears to be Spark 4.x-specific (the repo’s SQL resources only use this form undergluten-ut/spark40/spark41). As-is, this test is likely to fail when running the suite under older Spark profiles whereformat_numberonly accepts an integer decimal-places argument. Guard this block with a Spark-version check (or split into atestWithMinSparkVersion).
// (e.g. '#,###.##') has no matching signature, so it must fall back to vanilla Spark.
runQueryAndCompare(
"SELECT format_number(cast(l_quantity as double), '#,###.##') FROM lineitem limit 50") {
checkSparkPlan[ProjectExec]
}
|
thank you all. |
What changes are proposed in this pull request?
Velox already registers the sparksql
format_number(number, decimalPlaces)function (thousands separators, fixed decimal digits, HALF_EVEN rounding), so wire it up on the Gluten side.Unlike CAST(number AS VARCHAR), this adds thousands separators and fixed decimal places. Supports tinyint, smallint, integer, bigint, float, and double.
How was this patch tested?
New UTs.
Was this patch authored or co-authored using generative AI tooling?
Yes, Assisted-by: Claude:claude-opus-4-8