Skip to content

[SPARK-52716][SDP][SQL] Remove comment from Flow trait and references#51406

Closed
JiaqiWang18 wants to merge 1 commit into
apache:masterfrom
JiaqiWang18:SPARK-52716-remove-comment-from-flow-schema
Closed

[SPARK-52716][SDP][SQL] Remove comment from Flow trait and references#51406
JiaqiWang18 wants to merge 1 commit into
apache:masterfrom
JiaqiWang18:SPARK-52716-remove-comment-from-flow-schema

Conversation

@JiaqiWang18

@JiaqiWang18 JiaqiWang18 commented Jul 9, 2025

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

  • Removed the comment field from the Flow trait, child UnresolvedFlow and `ResolutionCompletedFlow
  • Removed all references of the comment field, including where it is being populated in SqlGraphRegistrationContext.scala
  • Update test SqlPipelineSuite.scala to remove assertion for the field. This is the only place that's accessing/modifying the field value.

Why are the changes needed?

In Spark Declarative Pipelines (SDP), the Flow trait has an unused comment field that is not being referenced anywhere.
Since there is no way for user to see flow comments as of now, this PR removes the field.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Updated unit test that references the field and made sure all other test passes.

Was this patch authored or co-authored using generative AI tooling?

No

@JiaqiWang18 JiaqiWang18 changed the title [WIP][SPARK-52716] Remove comment from Flow trait and references [WIP][SPARK-52716][SDP][SQL] Remove comment from Flow trait and references Jul 9, 2025
@JiaqiWang18 JiaqiWang18 changed the title [WIP][SPARK-52716][SDP][SQL] Remove comment from Flow trait and references [SPARK-52716][SDP][SQL] Remove comment from Flow trait and references Jul 9, 2025
@JiaqiWang18

Copy link
Copy Markdown
Contributor Author

@AnishMahto

@dongjoon-hyun dongjoon-hyun left a comment

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.

Thank you for making a PR, @JiaqiWang18 .

This feature is still under development in the time frame of Apache Spark 4.1.0. Are we sure that this is not going to be used in the future? It seems too early for me to decide.

Since there is no way for user to see flow comments as of now, this PR removes the field.

@dongjoon-hyun

Copy link
Copy Markdown
Member

cc @sryza , @HyukjinKwon , @peter-toth

@sryza sryza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As someone who helped author the original PR that added this, my understanding is that this was added accidentally (out of unneeded consistency with the Table class), so I think removing it is the right thing to do. We can add it back if comments become meaningful in the context of flows.

@JiaqiWang18

JiaqiWang18 commented Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

This feature is still under development in the time frame of Apache Spark 4.1.0. Are we sure that this is not going to be used in the future? It seems too early for me to decide.

Since there is no way for user to see flow comments as of now, this PR removes the field.

Yeah, I think SDP don't have a way to display this to the user. We can add it back in if it gets one

@dongjoon-hyun dongjoon-hyun left a comment

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.

+1, LGTM per the above discussion.

@HyukjinKwon

Copy link
Copy Markdown
Member

Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants