Skip to content

[CALCITE-6179] Support weekofmonth function format and add test#3600

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
caicancai:format_test
Dec 29, 2023
Merged

[CALCITE-6179] Support weekofmonth function format and add test#3600
mihaibudiu merged 1 commit intoapache:mainfrom
caicancai:format_test

Conversation

@caicancai
Copy link
Member

@caicancai caicancai commented Dec 26, 2023

@caicancai caicancai marked this pull request as draft December 26, 2023 15:35
@caicancai
Copy link
Member Author

caicancai commented Dec 26, 2023

Is the result of format affected by the jdk version?
https://github.com/apache/calcite/actions/runs/7330705153/job/19962233529

@macroguo-ghy
Copy link
Contributor

You need to log a JIRA case, because you have added new features.

@caicancai
Copy link
Member Author

You need to log a JIRA case, because you have added new features.

Thank you for your reply

@caicancai caicancai marked this pull request as ready for review December 27, 2023 13:41
@caicancai caicancai changed the title [MINOR] Add FormatElementEnumTests and add weekofmonth format [CALCITE-6179] Support weekofmonth function format and add test Dec 27, 2023
@caicancai caicancai requested a review from mihaibudiu December 28, 2023 00:31
Copy link
Contributor

@macroguo-ghy macroguo-ghy left a comment

Choose a reason for hiding this comment

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

Utilities for FormatModel in org.apache.calcite.util.format.FormatModels should be taken into consideration.

@caicancai
Copy link
Member Author

@macroguo-ghy If you have time, can you help review my code? Thank you.

@macroguo-ghy
Copy link
Contributor

@caicancai Did you miss my previous comment? I think you should add week of month to Format model for PostgreSQL. Please refer org.apache.calcite.util.format.FormatModels#POSTGRESQL and https://www.postgresql.org/docs/current/functions-formatting.html.

@caicancai
Copy link
Member Author

@caicancai Did you miss my previous comment? I think you should add week of month to Format model for PostgreSQL. Please refer org.apache.calcite.util.format.FormatModels#POSTGRESQL and https://www.postgresql.org/docs/current/functions-formatting.html.

@macroguo-ghy I didn't seem to understand it before, I'm sorry about that, I understand it now, thank you for your reivew

@sonarqubecloud
Copy link

Copy link
Contributor

@macroguo-ghy macroguo-ghy left a comment

Choose a reason for hiding this comment

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

+1

@macroguo-ghy macroguo-ghy added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Dec 29, 2023
@caicancai
Copy link
Member Author

@macroguo-ghy @mihaibudiu Good to merge?

@mihaibudiu mihaibudiu merged commit be297d0 into apache:main Dec 29, 2023
@caicancai caicancai deleted the format_test branch December 30, 2023 02:33
@caicancai
Copy link
Member Author

Thank you for the review

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

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants