fix(spark)!: derive the dialect from core's model and the runtime's functions - #1133
fix(spark)!: derive the dialect from core's model and the runtime's functions#1133nielspardon wants to merge 1 commit into
Conversation
f7dd4b9 to
cb4ab38
Compare
alexandrefimov
left a comment
There was a problem hiding this comment.
Read this against main at 961f83e, which now includes #1128 — that is where the one thing I would act on comes from.
max_precision for the three temporal types is the last restatement left in supportedTypes, and it is the kind this PR is about. Some(9) says what Util.MICROSECOND_PRECISION and the conversion guard already say, and they now say 6: #1128 pinned the type conversions at exactly microseconds and fed the generator from that constant. Rebasing conflicts in DialectGenerator.scala and spark_dialect.yaml, and resolving either in this branch's favour puts the 9 back — I tried it, and ./gradlew dialect regenerates the file with max_precision: 9 in three places, which fails core's SparkDialectParseTest.parsesPrecisionTypes with expected: <6> but was: <9>. CI catches it, so this is a heads-up for the rebase rather than a defect in what is here.
Separately, I checked the premise of the aggregate/window fix, since you note it has no test that can fail today. It holds: on main the generator reads COLLECTION.aggregateFunctions() where COLLECTION = EXTENSION_COLLECTION.merge(SparkImpls), while toAggregateFunction and toWindowFunction are built from EXTENSION_COLLECTION alone. So the advertised set really is the wider one, and the scalar side was already consistent because the generator used SparkScalarFunctions, which is the merged one on both sides.
cb4ab38 to
3207810
Compare
|
Rebased onto Thanks for checking the aggregate/window premise independently — that was the part I could not pin with a test. |
c58455a to
fca26d8
Compare
…unctions The generator restated things it could derive, and each restatement was a way for the published spark_dialect.yaml to drift without a test failing. Aggregate and window functions came from the collection merged with spark.yml while the runtime converters bind against the standard extensions only, so an aggregate added to spark.yml would be advertised and then fail with "Unable to find binding for call". All three sections now come from the collections SparkExtension hands the runtime, passed to the generator explicitly. The dependencies block was an unsorted Map, so its order was an artifact of one Scala version's String hashing; it is now built as a SortedMap from the URNs the emitted functions actually reference. A hand-maintained URN-to-alias map returned "" for an unmapped URN, emitting a source that dangled against dependencies while still validating against the schema, which declares source as a plain string; the alias is now derived from the URN's last segment. The dialect is emitted through io.substrait.dialect.Dialect rather than a parallel set of Scala case classes that typed enums as String, so the generator can no longer express a dialect core would reject. The published file is unchanged apart from key order: core's field order, dependencies sorted, and max_precision ahead of system_metadata. An ordering change cannot be caught by comparing the parsed models, since Dialect.dependencies is a Map, so the published text is now compared as text. Closes substrait-io#1087 BREAKING CHANGE: io.substrait.spark.utils.Dialect, SupportedType, TypeMetadata, FunctionMetadata and SupportedFunction are removed; the dialect is modelled by io.substrait.dialect.Dialect and friends. DialectGenerator.generate() returns io.substrait.dialect.Dialect, and the DialectGenerator class now takes the scalar, aggregate and window function collections it generates from.
fca26d8 to
666fe3d
Compare
The generator restated things it could derive, and each restatement was a way for the published
spark_dialect.yamlto drift without a test failing.Aggregate and window functions came from the collection merged with
spark.ymlwhile the runtime converters bind against the standard extensions only, so an aggregate added tospark.ymlwould be advertised and then fail with "Unable to find binding for call". All three sections now come from the collectionsSparkExtensionhands the runtime, passed to the generator explicitly.The
dependenciesblock was an unsortedMap, so its order was an artifact of one Scala version's String hashing; it is now built as aSortedMapfrom the URNs the emitted functions actually reference. A hand-maintained URN-to-alias map returned""for an unmapped URN, emitting asourcethat dangled againstdependencieswhile still validating against the schema, which declaressourceas a plain string; the alias is now derived from the URN's last segment, and two URNs deriving the same alias fail rather than one silently displacing the other.The dialect is emitted through
io.substrait.dialect.Dialectrather than a parallel set of Scala case classes that typed enums asString, so the generator can no longer express a dialect core would reject, and a dialect-schema field core gains no longer has to be added a second time before Spark can express it.The published file is unchanged apart from key order: core's field order,
dependenciessorted, andmax_precisionahead ofsystem_metadata. Comparing the parsed models cannot catch an ordering change, sinceDialect.dependenciesis aMap, so the published text is now compared as text — which in turn needs the file declared as aTestinput so that editing it invalidates the tests.Worth knowing while reviewing: the aggregate/window fix has no test that can fail today. With
spark.ymldeclaring no aggregate or window function, the merged collection and the standard collection are indistinguishable, so reverting that wiring alone breaks nothing observable. The guard checks the advertised aggregates and windows againstDefaultExtensionCatalog.DEFAULT_COLLECTIONdirectly rather than against what the generator was handed, so it arms the momentspark.ymlgains one — the same moment the bug would go live.dependenciesis still derived from function URNs only, so aUSER_DEFINEDsupported type would need its own alias folded in; the dangling-source test covers type sources too, so that would fail rather than ship.Two unreachable branches in the function probe went with the rewrite, one of them a
printlnaimed at the sameSystem.outthatmainwrites the dialect to.Closes #1087
BREAKING CHANGE:
io.substrait.spark.utils.Dialect,SupportedType,TypeMetadata,FunctionMetadataandSupportedFunctionare removed; the dialect is modelled byio.substrait.dialect.Dialectand friends.DialectGenerator.generate()returnsio.substrait.dialect.Dialect, and theDialectGeneratorclass now takes the scalar, aggregate and window function collections it generates from.