Skip to content

[SPARK-50831][SQL] Enable trimming collation by default#49510

Closed
stevomitric wants to merge 3 commits intoapache:masterfrom
stevomitric:stevomitric/enable-rtrim-default
Closed

[SPARK-50831][SQL] Enable trimming collation by default#49510
stevomitric wants to merge 3 commits intoapache:masterfrom
stevomitric:stevomitric/enable-rtrim-default

Conversation

@stevomitric
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Changed the sql config to enable the trimming collation by default.

Why are the changes needed?

As per the collation project plan.

Does this PR introduce any user-facing change?

Yes, the trimming collation will be enabled by default.

How was this patch tested?

Existing trimming tests.

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

No

@github-actions github-actions bot added the SQL label Jan 15, 2025
@stevomitric
Copy link
Copy Markdown
Contributor Author

cc @stefankandic and @dejankrak-db

Copy link
Copy Markdown
Contributor

@stefankandic stefankandic left a comment

Choose a reason for hiding this comment

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

LGTM, I would also like a confirmation from @jovanpavl-db just to double check that there are no blockers for enabling this by default

@jovanpavl-db
Copy link
Copy Markdown
Contributor

LGTM, I would also like a confirmation from @jovanpavl-db just to double check that there are no blockers for enabling this by default
Yes, ready to go.

Copy link
Copy Markdown
Contributor

@jovanpavl-db jovanpavl-db left a comment

Choose a reason for hiding this comment

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

Keep in mind that there are some tests added when introducing this feature flag that needs to be removed, take a look at:
https://github.com/apache/spark/pull/48222/files

dongjoon-hyun
dongjoon-hyun previously approved these changes Jan 15, 2025
.version("4.0.0")
.booleanConf
.createWithDefault(Utils.isTesting)
.createWithDefault(true)
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.

Oh, wait.

Do we need to revise the doc?

"Trim collation feature is under development and its use should be done under this" +
"feature flag. Trim collation trims trailing whitespaces from strings."

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.

The development is finished, isn't it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the message. cc @dongjoon-hyun

@dongjoon-hyun
Copy link
Copy Markdown
Member

Gentle ping, @stevomitric .

@MaxGekk MaxGekk self-requested a review January 16, 2025 08:41
@MaxGekk
Copy link
Copy Markdown
Member

MaxGekk commented Jan 16, 2025

+1, LGTM. Merging to master.
Thank you, @stevomitric and @dongjoon-hyun @jovanpavl-db @stefankandic for review.

@MaxGekk MaxGekk closed this in 96adcc4 Jan 16, 2025
@dongjoon-hyun
Copy link
Copy Markdown
Member

Thank you all!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants