Skip to content

fix(spark)!: derive the dialect from core's model and the runtime's functions #1133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nielspardon wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(spark)!: derive the dialect from core's model and the runtime's functions #1133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
666fe3d
fix(spark)!: derive the dialect from core's model and the runtime's f…
nielspardon Aug 19, 2026
350ce87
fix(spark): address review on the derived dialect generator
nielspardon Sep 3, 2026
4f5c32b
Merge branch 'main' into fix/dialect-generator-derived-model
nielspardon Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions spark/spark-3.4_2.12/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,13 @@ tasks {
test {
useJUnitPlatform { includeEngines("scalatest") }

// DialectSuite reads the published dialect, so a change to it has to invalidate the tests.
// Only the content matters, so ignore the path and keep the task cacheable across checkouts.
inputs
.file("../spark_dialect.yaml")
.withPropertyName("publishedDialect")
.withPathSensitivity(PathSensitivity.NONE)

// Set system properties for variant identification
systemProperty("spark.version", sparkVersion)
systemProperty("scala.version", scalaVersion)
Expand Down
7 changes: 7 additions & 0 deletions spark/spark-3.5_2.12/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ tasks {
test {
useJUnitPlatform { includeEngines("scalatest") }

// DialectSuite reads the published dialect, so a change to it has to invalidate the tests.
// Only the content matters, so ignore the path and keep the task cacheable across checkouts.
inputs
.file("../spark_dialect.yaml")
.withPropertyName("publishedDialect")
.withPathSensitivity(PathSensitivity.NONE)

// Set system properties for variant identification
systemProperty("spark.version", sparkVersion)
systemProperty("scala.version", scalaVersion)
Expand Down
7 changes: 7 additions & 0 deletions spark/spark-4.0_2.13/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,13 @@ tasks {
test {
useJUnitPlatform { includeEngines("scalatest") }

// DialectSuite reads the published dialect, so a change to it has to invalidate the tests.
// Only the content matters, so ignore the path and keep the task cacheable across checkouts.
inputs
.file("../spark_dialect.yaml")
.withPropertyName("publishedDialect")
.withPathSensitivity(PathSensitivity.NONE)
Comment on lines +168 to +171

@andrew-coleman andrew-coleman Sep 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dialect rewrites this same file, and nothing orders the two tasks: ./gradlew test dialect runs test first, so it can validate the pre-regeneration content, pass, and leave the file modified. Confirmed with --dry-run.

Declaring the file as an outputs.file on dialect is not enough on its own — Gradle does not infer the dependency from plain-String inputs.file/outputs.file pairs, and I verified test still ran first with the output declared. The ordering has to be explicit:

Suggested change
inputs
.file("../spark_dialect.yaml")
.withPropertyName("publishedDialect")
.withPathSensitivity(PathSensitivity.NONE)
inputs
.file("../spark_dialect.yaml")
.withPropertyName("publishedDialect")
.withPathSensitivity(PathSensitivity.NONE)
// The dialect task rewrites that same file and Gradle does not infer the ordering from the
// input/output declarations, so without this `./gradlew test dialect` validates the
// pre-regeneration content.
mustRunAfter("dialect")

Worth adding outputs.file("../spark_dialect.yaml") to the registration on line 149 as well, separately — that gets dialect up-to-date checking instead of re-running on every invocation:

tasks.register<JavaExec>("dialect") {
  classpath = java.sourceSets["main"].runtimeClasspath
  mainClass = "io.substrait.spark.utils.DialectGenerator"
  args = listOf("../spark_dialect.yaml")
  outputs.file("../spark_dialect.yaml")
}

I checked drift is still handled with that in place: corrupting the YAML makes the task re-run and repair it, so the only visible change is dialect reporting UP-TO-DATE when nothing moved.

Note this variant is the only one that needs mustRunAfter — dialect is registered here alone, so the same line in the 3.4/3.5 build files would fail on an unknown task. A short comment there explaining why the input is content-only would save the next reader the trip.


// Set system properties for variant identification
systemProperty("spark.version", sparkVersion)
systemProperty("scala.version", scalaVersion)
Expand Down
54 changes: 27 additions & 27 deletions spark/spark_dialect.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,18 @@
---
name: "Spark Dialect"
dependencies:
aggregate_approx: "extension:io.substrait:functions_aggregate_approx"
aggregate_generic: "extension:io.substrait:functions_aggregate_generic"
arithmetic: "extension:io.substrait:functions_arithmetic"
arithmetic_decimal: "extension:io.substrait:functions_arithmetic_decimal"
boolean: "extension:io.substrait:functions_boolean"
comparison: "extension:io.substrait:functions_comparison"
datetime: "extension:io.substrait:functions_datetime"
logarithmic: "extension:io.substrait:functions_logarithmic"
rounding: "extension:io.substrait:functions_rounding"
rounding_decimal: "extension:io.substrait:functions_rounding_decimal"
spark: "extension:substrait:spark"
string: "extension:io.substrait:functions_string"
supported_types:
- type: "I8"
system_metadata:
Expand Down Expand Up @@ -54,20 +67,20 @@ supported_types:
name: "BooleanType"
supported_as_column: true
- type: "PRECISION_TIMESTAMP"
max_precision: 6
system_metadata:
name: "TimestampNTZType"
supported_as_column: true
max_precision: 6
- type: "PRECISION_TIMESTAMP_TZ"
max_precision: 6
system_metadata:
name: "TimestampType"
supported_as_column: true
max_precision: 6
- type: "INTERVAL_DAY"
max_precision: 6
system_metadata:
name: "DayTimeIntervalType"
supported_as_column: true
max_precision: 6
- type: "INTERVAL_YEAR"
system_metadata:
name: "YearMonthIntervalType"
Expand All @@ -84,17 +97,6 @@ supported_types:
system_metadata:
name: "StructType"
supported_as_column: true
supported_expressions:
- "LITERAL"
- "SELECTION"
- "SCALAR_FUNCTION"
- "IF_THEN"
- "SINGULAR_OR_LIST"
- "CAST"
- expression: "SUBQUERY"
subquery_types:
- "SCALAR"
- "IN_PREDICATE"
supported_relations:
- "FILTER"
- "FETCH"
Expand Down Expand Up @@ -125,19 +127,17 @@ supported_relations:
- relation: "SET"
operations:
- "UNION_ALL"
dependencies:
rounding: "extension:io.substrait:functions_rounding"
comparison: "extension:io.substrait:functions_comparison"
logarithmic: "extension:io.substrait:functions_logarithmic"
datetime: "extension:io.substrait:functions_datetime"
arithmetic: "extension:io.substrait:functions_arithmetic"
aggregate_generic: "extension:io.substrait:functions_aggregate_generic"
boolean: "extension:io.substrait:functions_boolean"
aggregate_approx: "extension:io.substrait:functions_aggregate_approx"
rounding_decimal: "extension:io.substrait:functions_rounding_decimal"
string: "extension:io.substrait:functions_string"
spark: "extension:substrait:spark"
arithmetic_decimal: "extension:io.substrait:functions_arithmetic_decimal"
supported_expressions:
- "LITERAL"
- "SELECTION"
- "SCALAR_FUNCTION"
- "IF_THEN"
- "SINGULAR_OR_LIST"
- "CAST"
- expression: "SUBQUERY"
subquery_types:
- "SCALAR"
- "IN_PREDICATE"
supported_scalar_functions:
- source: "arithmetic"
name: "abs"
Expand Down
11 changes: 7 additions & 4 deletions spark/src/main/scala/io/substrait/spark/SparkExtension.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,13 @@ object SparkExtension {
ret.toSeq
}

val toAggregateFunction: ToAggregateFunction = ToAggregateFunction(
EXTENSION_COLLECTION.aggregateFunctions().asScala.toSeq)
lazy val StandardAggregateFunctions: Seq[SimpleExtension.AggregateFunctionVariant] =
EXTENSION_COLLECTION.aggregateFunctions().asScala.toSeq

val toWindowFunction: ToWindowFunction = ToWindowFunction(
lazy val StandardWindowFunctions: Seq[SimpleExtension.WindowFunctionVariant] =
EXTENSION_COLLECTION.windowFunctions().asScala.toSeq
Comment on lines +45 to 49

@andrew-coleman andrew-coleman Sep 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The narrowing makes the generator match the runtime, which is right. The side effect is that a spark.yml aggregate would now be silently absent from the dialect while ToSubstraitRel:108 still cannot bind it — the loud runtime failure survives, but the dialect no longer hints at why. Pointing these at COLLECTION instead would make both truthful.

If the narrowing is the deliberate choice, worth marking so the next person doesn't read the omission as an oversight:

Suggested change
lazy val StandardAggregateFunctions: Seq[SimpleExtension.AggregateFunctionVariant] =
EXTENSION_COLLECTION.aggregateFunctions().asScala.toSeq
val toWindowFunction: ToWindowFunction = ToWindowFunction(
lazy val StandardWindowFunctions: Seq[SimpleExtension.WindowFunctionVariant] =
EXTENSION_COLLECTION.windowFunctions().asScala.toSeq
/**
* Standard extensions only: `spark.yml` declares no aggregates or windows, and
* [[toAggregateFunction]] / [[toWindowFunction]] bind against these same collections, so a
* `spark.yml` aggregate would be neither advertised in the dialect nor bindable at runtime.
*/
lazy val StandardAggregateFunctions: Seq[SimpleExtension.AggregateFunctionVariant] =
EXTENSION_COLLECTION.aggregateFunctions().asScala.toSeq
/** @see [[StandardAggregateFunctions]] */
lazy val StandardWindowFunctions: Seq[SimpleExtension.WindowFunctionVariant] =
EXTENSION_COLLECTION.windowFunctions().asScala.toSeq

)

val toAggregateFunction: ToAggregateFunction = ToAggregateFunction(StandardAggregateFunctions)

val toWindowFunction: ToWindowFunction = ToWindowFunction(StandardWindowFunctions)
}
Loading
Loading