Build: Apply spotless for scala code #8023

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.

nastra · 2025-06-23T16:48:08Z

.baseline/scala/.scala212fmt.conf

+
+version = 3.9.7
+
+align = none


I haven't checked but are these settings resulting in code style that it very close to the Apache Spark codebase?

Yes, the spark config is here: https://github.com/apache/spark/blob/master/dev/.scalafmt.conf.

The differences:

# spark maxColumn = 98 version = 3.8.6

# iceberg docstrings.wrap = false # suggestion from comments maxColumn = 100 rewrite.rules = [Imports] version = 3.9.7

for learning, why do we deviate for [Imports]

Removed the [Imports] to keep it aligned with Spark. We could add the following if needed:

// https://scalameta.org/scalafmt/docs/configuration.html#imports rewrite.rules = [Imports] rewrite.imports.sort = original

nastra · 2025-06-24T07:24:20Z

.baseline/scala/.scala212fmt.conf

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


given that we can fully control this (and since we're deviating from Spark anyway) I think it's fine to set this to 120 or 140 even. Let's see what others think.

nastra

this LGTM, I think we can discuss what the final maxColumn value should be (e.g. 120 or 140 instead of 100).
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 · 2025-06-24T07:28:31Z

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

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.

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

nastra reviewed Jun 23, 2025

View reviewed changes

nastra reviewed Jun 24, 2025

View reviewed changes

nastra approved these changes Jun 24, 2025

View reviewed changes

nastra requested a review from aokolnychyi June 24, 2025 07:27

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 added 2 commits June 28, 2025 13:39

Add scalafmt and config for spotless

2629e3b

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

ebe4130

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

Build: Apply spotless for scala code #8023

Are you sure you want to change the base?

Build: Apply spotless for scala code #8023

Uh oh!

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!

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!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

nastra commented Jun 24, 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!

Reviewers

Assignees

Labels

Projects

Milestone