Build: Apply spotless for scala code by ConeyLiu · Pull Request #8023 · apache/iceberg

ConeyLiu · 2023-07-09T08:58:45Z

Closes #7695.

ConeyLiu · 2023-07-09T09:05:16Z

.baseline/checkstyle/.scalafmt.conf

+# specific language governing permissions and limitations
+# under the License.
+
+align = none


This is modified from spark

ConeyLiu · 2023-07-09T09:05:47Z

.baseline/checkstyle/.scalafmt.conf

+}
+danglingParentheses.preset = false
+docstrings.style = Asterisk
+maxColumn = 100


Set to 100 to align java code.

just curious, what's the default value? For Java the default is mandated by the Google Java Format and set to 100

The default is 80, docs at here: https://scalameta.org/scalafmt/docs/configuration.html#maxcolumn

.../scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala

...-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/MergeIntoExec.scala

...sions/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentAlignmentSupport.scala

zinking · 2023-07-12T01:25:59Z

@Fokko are we going to force styles on scala as well? not sure why this isn't done when enforcing styles on java.
if the answer is yes, guess we need to do this early instead of late.

ConeyLiu · 2023-07-18T02:57:10Z

Hi, @nastra @zinking @Fokko would you mind taking another look at this? Thanks in advance.

ConeyLiu · 2023-07-18T03:00:05Z

Also cc @jackye1995 @amogh-jahagirdar who have disscuessed this at #6736

nastra · 2023-07-18T06:57:11Z

@ConeyLiu I think this is a great topic to mention on the DEV mailist list & Iceberg Sync, so that people are aware about this effort

ConeyLiu · 2023-07-18T09:06:56Z

Thanks @nastra, raised the discussion at the dev mail list.

zinking · 2023-07-19T05:03:04Z

LGTM now, I'd suggest let's get this style in early.

ajantha-bhat · 2023-12-18T10:34:14Z

Just linking the conclusion here as I was searching for it.
https://lists.apache.org/thread/sv70lr0bwl9jmxtzvho2ml5xcrcpzf3b

github-actions · 2024-09-06T00:14:35Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

github-actions · 2024-09-19T00:14:28Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

nastra · 2025-05-14T15:21:21Z

@ConeyLiu are you interested in continuing work on this?

ConeyLiu · 2025-05-15T03:03:10Z

Sure, I will rebase it these days.

github-actions · 2025-06-15T00:19:59Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

ConeyLiu · 2025-06-24T11:49:31Z

Also it would be good to update https://github.com/apache/iceberg/blob/8e897f1b68c38c6352f23d636c320e3754d58eef/site/docs/contribute.md#setting-up-ide-and-code-style and mention how to set up IntelliJ with Scalafmt so that formatting inside IntelliJ produces the same formatting results across cmd line (./gradlew spotlessApply) and the IDE

@nastra Does this need a separate commit or rebased into the first commit 'Add scalafmt and config for spotless'

ConeyLiu · 2025-06-24T11:50:52Z

could you please raise awareness about this on the DEV mailing list so that people have a chance to review this?

Done it.

nastra · 2025-06-24T13:04:09Z

Also it would be good to update https://github.com/apache/iceberg/blob/8e897f1b68c38c6352f23d636c320e3754d58eef/site/docs/contribute.md#setting-up-ide-and-code-style and mention how to set up IntelliJ with Scalafmt so that formatting inside IntelliJ produces the same formatting results across cmd line (./gradlew spotlessApply) and the IDE

@nastra Does this need a separate commit or rebased into the first commit 'Add scalafmt and config for spotless'

I think adding this into the first commit should be fine

szehon-ho · 2025-06-24T17:44:53Z

.baseline/scala/.scala212fmt.conf

+
+version = 3.9.7
+
+align = none


for learning, why do we deviate for [Imports]

szehon-ho · 2025-06-24T17:47:23Z

...spark-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CallExec.scala

-    output: Seq[Attribute],
-    procedure: Procedure,
-    input: InternalRow) extends LeafV2CommandExec {
+case class CallExec(output: Seq[Attribute], procedure: Procedure, input: InternalRow)


nit: can we keep the old one to be more like Spark and be more readable (or does it violate scalafmt because of maxColumns?)

This is because all parameters can be placed in a single line without exceeding the maximum line limit; if the limit is exceeded, they will be stored in multiple columns. I checked the Spark code, and it uses a similar approach.

site/docs/contribute.md

manuzhang · 2025-09-22T16:50:30Z

.../scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala

 import scala.jdk.CollectionConverters._

-class IcebergSparkSqlExtensionsParser(delegate: ParserInterface) extends ParserInterface with ExtendedParser {
+class IcebergSparkSqlExtensionsParser(delegate: ParserInterface)


The import org.apache.iceberg.spark.procedures.SparkProcedures is not used in this class. Should it be removed by spotless?

jackylee-ch · 2025-10-14T02:12:12Z

@singhpk234 Based on the issue and PR in Spotless, the unused imports aren’t handled. We need a separate Scalafix rule to take care of that.

nastra · 2025-11-27T07:17:48Z

@ConeyLiu where are we with this PR? Could you please rebase it so that we can get it in?

ConeyLiu · 2025-11-28T01:56:12Z

I can rebase it today or tomorrow.

kevinjqliu

LGTM, thanks for adding this.

i tried this out locally. Looks like the spark scalafmt has been modified, maybe we should align with the new configs

kevinjqliu · 2025-11-29T17:21:33Z

baseline.gradle

+      // Configure different scalafmt rules for specific Scala version
+      if (project.name.startsWith("iceberg-spark") && project.name.endsWith("2.13")) {
+        scala {
+          target 'src/main/scala/**/*.scala', 'src/test/scala/**/*.scala', 'src/testFixtures/scala/**/*.scala'
+          scalafmt("3.9.7").configFile("$rootDir/.baseline/scala/.scala213fmt.conf")
+          licenseHeaderFile "$rootDir/.baseline/copyright/copyright-header-java.txt", "package"
+        }
+      } else if (project.name.startsWith("iceberg-spark") && project.name.endsWith("2.12")) {
+        scala {
+          target 'src/main/scala/**/*.scala', 'src/test/scala/**/*.scala', 'src/testFixtures/scala/**/*.scala'
+          scalafmt("3.9.7").configFile("$rootDir/.baseline/scala/.scala212fmt.conf")
+          licenseHeaderFile "$rootDir/.baseline/copyright/copyright-header-java.txt", "package"
+        }
+      }


Suggested change

// Configure different scalafmt rules for specific Scala version

if (project.name.startsWith("iceberg-spark") && project.name.endsWith("2.13")) {

scala {

target 'src/main/scala/**/*.scala', 'src/test/scala/**/*.scala', 'src/testFixtures/scala/**/*.scala'

scalafmt("3.9.7").configFile("$rootDir/.baseline/scala/.scala213fmt.conf")

licenseHeaderFile "$rootDir/.baseline/copyright/copyright-header-java.txt", "package"

}

} else if (project.name.startsWith("iceberg-spark") && project.name.endsWith("2.12")) {

scala {

target 'src/main/scala/**/*.scala', 'src/test/scala/**/*.scala', 'src/testFixtures/scala/**/*.scala'

scalafmt("3.9.7").configFile("$rootDir/.baseline/scala/.scala212fmt.conf")

licenseHeaderFile "$rootDir/.baseline/copyright/copyright-header-java.txt", "package"

}

}

// Configure different scalafmt rules for specific Scala version

if (project.name.startsWith("iceberg-spark") && project.name.endsWith("2.13")) {

scala {

target 'src/**/*.scala'

scalafmt("3.9.7").configFile("$rootDir/.baseline/scala/.scala213fmt.conf")

licenseHeaderFile "$rootDir/.baseline/copyright/copyright-header-java.txt", "package"

}

} else if (project.name.startsWith("iceberg-spark") && project.name.endsWith("2.12")) {

scala {

target 'src/**/*.scala'

scalafmt("3.9.7").configFile("$rootDir/.baseline/scala/.scala212fmt.conf")

licenseHeaderFile "$rootDir/.baseline/copyright/copyright-header-java.txt", "package"

}

}

nit capture all .scala files in case more folders are added in the future

Thanks @kevinjqliu, updated

kevinjqliu · 2025-11-29T17:27:47Z

.baseline/scala/.scala212fmt.conf

+
+version = 3.9.7
+
+align = none


rewrite.rules

has been removed from https://github.com/apache/spark/blob/f5b9ea8103dd1f37c6f0cea0692d7bc5b50b778c/dev/.scalafmt.conf

the only difference now is version (3.8.6 vs 3.9.7) and maxColumn (98 vs 100) and docstrings.wrap

jackylee-ch · 2025-12-02T11:54:35Z

baseline.gradle

      }
+
+      // Configure different scalafmt rules for specific Scala version
+      if (project.name.startsWith("iceberg-spark") && project.name.endsWith("2.13")) {


Do we need to check iceberg-spark? It only affects the Scala code, right? Also, should we get the scalaVersion from the System.getProperty rather than from the project name?

should we get the scalaVersion from the System.getProperty rather than from the project name?

i think thats a good idea, we do it here already

cc @ConeyLiu

I'll follow up on this these days

Submitted #14798

kevinjqliu · 2025-12-02T16:51:00Z

Thank you for the PR @ConeyLiu and thanks everyone for the review.
Merging this in to prevent more conflicts. We can continue to iterate in a follow up PR 😄

github-actions bot added spark INFRA build labels Jul 9, 2023

ConeyLiu commented Jul 9, 2023

View reviewed changes

zinking reviewed Jul 12, 2023

View reviewed changes

.../scala/org/apache/spark/sql/catalyst/parser/extensions/IcebergSparkSqlExtensionsParser.scala Outdated Show resolved Hide resolved

zinking reviewed Jul 12, 2023

View reviewed changes

...-extensions/src/main/scala/org/apache/spark/sql/execution/datasources/v2/MergeIntoExec.scala Outdated Show resolved Hide resolved

zinking reviewed Jul 12, 2023

View reviewed changes

...sions/src/main/scala/org/apache/spark/sql/catalyst/analysis/AssignmentAlignmentSupport.scala Outdated Show resolved Hide resolved

ConeyLiu force-pushed the scala-spotless branch from 04f9bc2 to 01e03e4 Compare July 18, 2023 02:52

github-actions bot added the stale label Sep 6, 2024

github-actions bot closed this Sep 19, 2024

nastra reopened this May 14, 2025

github-actions bot removed the stale label May 15, 2025

github-actions bot added the stale label Jun 15, 2025

ConeyLiu force-pushed the scala-spotless branch from 01e03e4 to 4f0d191 Compare June 17, 2025 06:44

Fokko added not-stale and removed stale labels Jun 17, 2025

Fokko approved these changes Jun 17, 2025

View reviewed changes

szehon-ho approved these changes Jun 24, 2025

View reviewed changes

ConeyLiu force-pushed the scala-spotless branch from 4f0d191 to 5557644 Compare June 26, 2025 14:53

github-actions bot added the docs label Jun 26, 2025

ConeyLiu force-pushed the scala-spotless branch from 5557644 to d76ed92 Compare June 26, 2025 14:59

ConeyLiu commented Jun 26, 2025

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

nastra reviewed Jun 27, 2025

View reviewed changes

site/docs/contribute.md Outdated Show resolved Hide resolved

ConeyLiu force-pushed the scala-spotless branch from af75869 to ebe4130 Compare June 28, 2025 05:41

nastra requested review from RussellSpitzer, amogh-jahagirdar and danielcweeks July 3, 2025 09:01

manuzhang reviewed Sep 22, 2025

View reviewed changes

singhpk234 mentioned this pull request Oct 13, 2025

Spark: Removed the unused imports in Scala Files #14311

Merged

ConeyLiu force-pushed the scala-spotless branch 2 times, most recently from a495530 to 4633c41 Compare November 29, 2025 13:56

kevinjqliu approved these changes Nov 29, 2025

View reviewed changes

ConeyLiu added 2 commits December 2, 2025 10:56

Add scalafmt and config for spotless

e9d7376

Apply spotless for scala code: './gradlew spotlessApply -DallModules'

64a6733

ConeyLiu force-pushed the scala-spotless branch from 4633c41 to 64a6733 Compare December 2, 2025 02:58

jackylee-ch reviewed Dec 2, 2025

View reviewed changes

kevinjqliu merged commit 166a6eb into apache:main Dec 2, 2025
47 checks passed

jbewing mentioned this pull request Dec 2, 2025

Spark: Analyze but don't optimize view body during creation #14681

Merged

ConeyLiu mentioned this pull request Dec 8, 2025

Build: Improvements around applying spotless for Scala #14798

Merged

thomaschow pushed a commit to thomaschow/iceberg that referenced this pull request Jan 19, 2026

Build: Apply spotless for scala code (apache#8023)

d426690

Conversation

ConeyLiu commented Jul 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zinking commented Jul 12, 2023

Uh oh!

ConeyLiu commented Jul 18, 2023

Uh oh!

ConeyLiu commented Jul 18, 2023

Uh oh!

nastra commented Jul 18, 2023

Uh oh!

ConeyLiu commented Jul 18, 2023

Uh oh!

zinking commented Jul 19, 2023

Uh oh!

ajantha-bhat commented Dec 18, 2023

Uh oh!

github-actions bot commented Sep 6, 2024

Uh oh!

github-actions bot commented Sep 19, 2024

Uh oh!

nastra commented May 14, 2025

Uh oh!

ConeyLiu commented May 15, 2025

Uh oh!

github-actions bot commented Jun 15, 2025

Uh oh!

ConeyLiu commented Jun 24, 2025

Uh oh!

ConeyLiu commented Jun 24, 2025

Uh oh!

nastra commented Jun 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackylee-ch commented Oct 14, 2025

Uh oh!

nastra commented Nov 27, 2025

Uh oh!

ConeyLiu commented Nov 28, 2025

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu commented Dec 2, 2025

Uh oh!