Derive default Scala cluster runtime from DBR version - #6565
Draft
rugpanov wants to merge 1 commit into
Draft
Conversation
Collaborator
Integration test reportCommit: 2e3f010
Top 4 slowest tests (at least 2 minutes):
|
rugpanov
force-pushed
the
default-scala-version-sync
branch
3 times, most recently
from
September 8, 2026 11:36
ae79520 to
0395e1f
Compare
rugpanov
force-pushed
the
default-scala-version-sync
branch
from
September 8, 2026 11:54
0395e1f to
2e3f010
Compare
anton-107
approved these changes
Sep 9, 2026
anton-107
left a comment
Contributor
There was a problem hiding this comment.
Approving — reviewed end-to-end; clean, low-risk refactor with good test coverage.
Summary
Replaces the hardcoded classic-cluster spark_version: 17.3.x-scala2.13 with a spark_version macro composed from the existing dbr_version and scala_major_minor_version macros. Rendered output is byte-identical (hence no existing golden changes), and it closes a real drift hazard: bumping dbr_version previously moved only the DB Connect client and left the cluster runtime stale.
What I checked:
- Composing
spark_versionfrom the two macrosdefault-scalaalready maintains — rather than introducing a third pin — is the right design and appropriate here, since both values are needed independently inbuild.sbt. - The new acceptance test faithfully follows the
dbt-sql-classicprecedent (Phase = 1+ diff against the sibling serverless golden via$TESTDIR); the phase gating inacceptance_test.gomakes the cross-test dependency sound.out.test.tomlfield order matches the generator, and"standard cluster"is a valid schema enum. - Unit tests are well-factored: the composition test and the wiring test are complementary.
- Macro whitespace renders cleanly, guarded by the explicit
TrimSpaceassertion.
Non-blocking comments
- The first-paragraph rules-doc edit (
cli-pipelines/ "defaultis not shipped") is accurate but logically independent of this change — consider splitting it per the repo's "keep each PR focused" convention, or at least note it in the description. TestDefaultScalaRenderedVersionsUseMacrosuses99.9.x-scala9.9as the fakespark_version, which mimics the real derived format and can read as if it tests derivation (it actually tests wiring). A clearly-synthetic sentinel would make the intent unambiguous.
Draft and currently behind main — rebase before merge.
Reviewed by Isaac (Claude Code).
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.
Changes
Why
The DB Connect dependency already follows dbr_version, but the classic job template duplicated its runtime as a literal. Updating only the macro could therefore produce a client that cannot connect to the generated cluster.
Follow-up to #6407.
Tests
This PR was written by Codex.