[SPARK-51187][SQL][SS] Implement the graceful deprecation of incorrect config introduced in SPARK-49699 #49983

HeartSaVioR · 2025-02-17T07:55:37Z

What changes were proposed in this pull request?

This PR proposes to implement the graceful deprecation of incorrect config introduced in SPARK-49699.

SPARK-49699 was included in Spark 3.5.4, hence we can't simply rename to fix the issue.

Also, since the incorrect config is logged in offset log in streaming query, the fix isn't just easy like adding withAlternative and done. We need to manually handle the case where offset log contains the incorrect config, and set the value of incorrect config in the offset log into the new config. Once a single microbatch has planned after the restart (hence the above logic is applied), offset log will contain the "new" config and it will no longer refer to the incorrect config.

That said, we can remove the incorrect config in the Spark version which we are confident that there will be no case users will upgrade from Spark 3.5.4 to that version.

Why are the changes needed?

We released an incorrect config and we want to rename it properly. While renaming, we don't also want to have any breakage on the existing streaming query.

Does this PR introduce any user-facing change?

No. That is what this PR is aiming for.

How was this patch tested?

New UT.

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

No.

…anPruneStreamingSubplan" This reverts commit decb677.

…he new one

HeartSaVioR · 2025-02-17T07:56:42Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -4115,6 +4115,7 @@ object SQLConf {
      .doc("Allow PruneFilters to remove streaming subplans when we encounter a false filter. " +
        "This flag is to restore prior buggy behavior for broken pipelines.")
      .version("4.0.0")
+      .withAlternative("spark.databricks.sql.optimizer.pruneFiltersCanPruneStreamingSubplan")


@dongjoon-hyun
Should we add this to DeprecatedConfig as well? Also, should we also file a JIRA ticket for removing?

It's no harm to add it. When we remove the config in the future (throw an error if it's set), we need a ticket.

HeartSaVioR · 2025-02-17T07:57:14Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/OffsetSeq.scala

+    // metadata in the offset log may have this if the batch ran from Spark 3.5.4.
+    // We need to pick the value from the metadata and set it in the new config.
+    // This also leads the further batches to have a correct config in the offset log.
+    metadata.conf.get("spark.databricks.sql.optimizer.pruneFiltersCanPruneStreamingSubplan") match {


I'll add TODO with mentioning removal when we want to file JIRA ticket for removal immediately.

HeartSaVioR · 2025-02-17T07:58:46Z

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingQuerySuite.scala

+              "value of the fixed config")
+
+          val offsetLog = new OffsetSeqLog(spark, new File(dir, "offsets").getCanonicalPath)
+          def checkConfigFromMetadata(batchId: Long, expectCorrectConfig: Boolean): Unit = {


I know this is a bit hard to understand from non-streaming expert folks. Please let me know if you need some further explanation to justify the logic.

HeartSaVioR · 2025-02-17T07:59:19Z

I'm going to submit PRs for 4.0/3.5 as well.

HeartSaVioR · 2025-02-17T08:03:23Z

cc. @dongjoon-hyun @HyukjinKwon Please take a look. Thanks!
cc. @cloud-fan for visibility of the fix for blocker issue

HeartSaVioR · 2025-02-17T08:03:53Z

[SPARK-51187][SQL][SS][4.0] Implement the graceful deprecation of incorrect config introduced in SPARK-49699 #49984 (4.0)
[SPARK-51187][SQL][SS][3.5] Implement the graceful deprecation of incorrect config introduced in SPARK-49699 #49985 (3.5)

dongjoon-hyun

Sorry for late response.

I was busy for internal preparing of Spark 4.0.0.

For this one, shall we do this at Apache Spark 3.5.5 while keeping Apache Spark 4.0.0 from from spark.databricks.*?

If you are busy, I can volunteer the release manager for Apache Spark 3.5.5.

When we release Apache Spark 3.5.5 this month, Apache Spark 4.0.0 can be free of spark.databricks.*.

WDYT, @HeartSaVioR and @cloud-fan ?

dongjoon-hyun · 2025-02-17T22:04:34Z

I sent an email for further discussion

https://lists.apache.org/thread/qwxb21g5xjl7xfp4rozqmg1g0ndfw2jd

HeartSaVioR · 2025-02-17T22:15:26Z

@dongjoon-hyun

Let me clarify a bit.

I have claimed that the config is not something user (even admin) would understand what it is and try to flip. That said, removing this config does not matter to me at all and I'm +1 to remove in Spark 4.0.0 (with argument that we can remove this in 3.5.5 as well).
The only issue is for users who ever ran their query in Spark 3.5.4, because the incorrect config is put to offset log and we shouldn't ignore this. I've added the logic to migrate to the incorrect config to new config when we read from offset log (and offset log for further microbatches will follow the new config). This logic should remain for multiple minor releases, so while we can discontinue supporting the incorrect config, we can't remove the incorrect config "key" from the codebase for multiple minor releases.

Please let me know if you are not on the same page in above, or have any question for this. Thanks!

If we are on the same page, I'll remove the config in master/4.0 PR and deprecate the config in 3.5.

HeartSaVioR · 2025-02-17T22:20:49Z

W.r.t. release manager for Spark 3.5.5, either is fine for me. I'm happy to take the step if you prefer to let me take it.

cloud-fan · 2025-02-18T03:23:56Z

If we have this grace handling in 3.5.5, we should have it in 4.0.0 as well, otherwise it's a breaking changing in 4.0.

HeartSaVioR · 2025-02-18T04:30:42Z

The migration logic can't be removed anytime soon. I'd say we could only remove it in 4.2 or so (conservatively speaking, 5.0). My all three PRs contain the migration logic.

I guess the main point here is when we can stop allowing users to specify the incorrect config by themselves. I have been arguing that users would never set this manually, but I'm open to being conservative on this as well.

HeartSaVioR · 2025-02-23T03:02:25Z

So according to recent discussion, I'm going to remove the withAlternative in this (4.1) and 4.0 PR, but should still need to persist the migration logic for a while. I hope we are on the same page on the necessity of migration logic for a while.
@dongjoon-hyun @cloud-fan

dongjoon-hyun · 2025-02-23T03:11:23Z

Technically, we didn't make an agreement on Apache Spark 4.0.0 behavior because there are two alternatives still.

One way is this approach which I also agree that technically correct, @HeartSaVioR .
The second one is to add a migration document simply for the users to ask to upgrade from Spark 3.5.x line to 3.5.5 or the above first before Apache Spark 4.0.0.

If you don't mind, I'd like to propose to wait until we finish Apache Spark 3.5.5 release.

HeartSaVioR · 2025-02-23T03:17:53Z

I'm really not sure 2 is a realistic alternative. We shouldn't assume users to be very close to hear Spark community news. My experience so far is telling me that users even don't read the doc.

Another point for me is that users do not really care about whether the config contains the vendor name or not. This is really a concern from ASF hat on (for sure, I agree this is problematic with one of PMC members in ASF project hat on), but I'm sure it's not a huge concern from users worth upgrading the version. So if we tell users to upgrade the Spark version just because of this problematic config, I'm very sure that they won't upgrade. (We shouldn't also lie to users that there is security concern or so.) We shouldn't pretend that it will be successful if we enforce users to upgrade to specific version.

cloud-fan · 2025-02-24T13:57:14Z

have we merged this graceful deprecation in branch 3.5?

HeartSaVioR · 2025-02-25T07:24:59Z

@cloud-fan

have we merged this graceful deprecation in branch 3.5?

Yes, that is merged. It's still a blocker for Spark 4.0.0 though.

@dongjoon-hyun

If you don't mind, I'd like to propose to wait until we finish Apache Spark 3.5.5 release.

Would you mind explaining why this has to be coupled with Spark 3.5.5, as discussion for 4.1/4.0 can always happen in parallel?

I sincerely disagree that the option 2 is viable for us. This builds a huge limitation of SS users for Spark 3.5.4 on upgrading (they have to touch Spark 3.5.x before upgrading to 4.0.0), to achieve our goal of removing the "string" of incorrect config immediately. Again, this is just a string because it's only for migration logic and users won't be able to set the config manually. Why not just be more open for Spark 3.5.4 users to have a range of upgrade path? I'll ensure I remove the migration logic and finally remove the string of the config in certain Spark version, which I prefer to have it in 4.1 or even longer term.

If you'd like to hear more voices, I'm happy to post the discussion to dev@. I just don't want this change to be coupled with non-relevant release.

dongjoon-hyun · 2025-03-04T03:09:30Z

Yes, please. Thank you, @HeartSaVioR .

If you'd like to hear more voices, I'm happy to post the discussion to dev@

HeartSaVioR · 2025-03-04T05:19:44Z

Thanks, posted to dev@. - https://lists.apache.org/thread/xzk9729lsmo397crdtk14f74g8cyv4sr

According to the community vote, this becomes stale.

github-actions · 2025-06-22T00:32:25Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

HeartSaVioR added 5 commits February 13, 2025 11:53

Revert "[SPARK-51172][SS] Rename to spark.sql.optimizer.pruneFiltersC…

0e1cfdd

…anPruneStreamingSubplan" This reverts commit decb677.

Fix to migrate the incorrect config we introduced in SPARK-49699 to t…

22cb5bb

…he new one

misc fix

37c52c0

fix offsetlog as they won't be a result of Spark 4.0.0 after this fix

6661426

remove FIXME

8e5dde4

HeartSaVioR self-assigned this Feb 17, 2025

github-actions bot added SQL STRUCTURED STREAMING labels Feb 17, 2025

HeartSaVioR commented Feb 17, 2025

View reviewed changes

revert unnecessary fix

077fc41

dongjoon-hyun previously requested changes Feb 17, 2025

View reviewed changes

dongjoon-hyun mentioned this pull request Feb 17, 2025

[SPARK-51187][SQL][SS][4.0] Implement the graceful deprecation of incorrect config introduced in SPARK-49699 #49984

Closed

HeartSaVioR added 2 commits February 23, 2025 12:03

remove alternative config

bcce8ac

also adding this to the migration guide for removal of config

0e03656

HeartSaVioR requested a review from dongjoon-hyun February 23, 2025 03:10

github-actions bot added the DOCS label Feb 23, 2025

HeartSaVioR requested a review from cloud-fan February 23, 2025 03:10

github-actions bot added the Stale label Jun 22, 2025

github-actions bot closed this Jun 23, 2025

[SPARK-51187][SQL][SS] Implement the graceful deprecation of incorrect config introduced in SPARK-49699 #49983

[SPARK-51187][SQL][SS] Implement the graceful deprecation of incorrect config introduced in SPARK-49699 #49983

Uh oh!

Conversation

HeartSaVioR commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Uh oh!

HeartSaVioR Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

cloud-fan Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Feb 17, 2025

Uh oh!

HeartSaVioR commented Feb 17, 2025

Uh oh!

HeartSaVioR commented Feb 17, 2025

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Feb 17, 2025

Uh oh!

HeartSaVioR commented Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeartSaVioR commented Feb 17, 2025

Uh oh!

cloud-fan commented Feb 18, 2025

Uh oh!

HeartSaVioR commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeartSaVioR commented Feb 23, 2025

Uh oh!

dongjoon-hyun commented Feb 23, 2025

Uh oh!

HeartSaVioR commented Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Feb 24, 2025

Uh oh!

HeartSaVioR commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Mar 4, 2025

Uh oh!

HeartSaVioR commented Mar 4, 2025

Uh oh!

github-actions bot commented Jun 22, 2025

Uh oh!

Uh oh!

HeartSaVioR commented Feb 17, 2025 •

edited

Loading

HeartSaVioR commented Feb 17, 2025 •

edited

Loading

HeartSaVioR commented Feb 18, 2025 •

edited

Loading

HeartSaVioR commented Feb 23, 2025 •

edited

Loading

HeartSaVioR commented Feb 25, 2025 •

edited

Loading