Skip to content

Conversation

@wForget
Copy link
Contributor

@wForget wForget commented Nov 17, 2025

In Velox, e was incorrectly used as the dayOfWeek letter in SimpleDateTimeFormatter.

case 'e':
builder.appendDayOfWeek1Based(count);
break;

This PR fixes this by using the letter u to be consistent with Java.
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/text/SimpleDateFormat.html

Closes #15516.

@netlify
Copy link

netlify bot commented Nov 17, 2025

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 8d8edf9
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/691d878ec7d166000830a4e4

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 17, 2025
@wForget
Copy link
Contributor Author

wForget commented Nov 17, 2025

@NEUpanning Could you please take a look?

@NEUpanning
Copy link
Contributor

LGTM, relates to #10354.

@wForget
Copy link
Contributor Author

wForget commented Nov 19, 2025

@rui-mo @jinchengchenghh Could you please take a look?

Copy link
Collaborator

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

@wForget Thanks for the fix. Does the letter differ across Java versions? Since this is a fix in "functions/lib", could it have any impact on Presto’s behavior?

@wForget
Copy link
Contributor Author

wForget commented Nov 19, 2025

@wForget Thanks for the fix. Does the letter differ across Java versions?

I checked java 8/17/21 and all of them use u to represent dayOfWeek.

Since this is a fix in "functions/lib", could it have any impact on Presto’s behavior?

It seems not, buildSimpleDateTimeFormatter is only used by spark.

Copy link
Contributor

@zml1206 zml1206 left a comment

Choose a reason for hiding this comment

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

After it was merged, #15535 will rebase.

@rui-mo
Copy link
Collaborator

rui-mo commented Nov 20, 2025

Created #15582 to track the workflow failure.

@rui-mo rui-mo changed the title fix: Correct dayOfWeek letter for SimpleDateTimeFormatter fix: Correct dayOfWeek letter for SimpleDateTimeFormatter Nov 20, 2025
@rui-mo rui-mo added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 20, 2025
@meta-codesync
Copy link

meta-codesync bot commented Nov 21, 2025

@kagamiori has imported this pull request. If you are a Meta employee, you can view this in D87597881.

@meta-codesync
Copy link

meta-codesync bot commented Nov 22, 2025

@kagamiori merged this pull request in ffa685d.

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DayOfWeek letter of SimpleDateTimeFormatter is incorrect

5 participants