Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement ADD_MONTH function #1341

Merged
merged 40 commits into from Sep 2, 2023
Merged

Implement ADD_MONTH function #1341

merged 40 commits into from Sep 2, 2023

Conversation

kite707
Copy link
Contributor

@kite707 kite707 commented Jul 28, 2023 •
edited

resolve #1291

@panarch panarch added the enhancement New feature or request label Jul 29, 2023
@kite707 kite707 marked this pull request as ready for review August 21, 2023 08:39
@kite707 kite707 changed the title Imple add date Imple add month Aug 21, 2023
@kite707 kite707 changed the title Imple add month Imple <mark>ADD_MONTH</mark> function Aug 21, 2023
@kite707 kite707 changed the title Imple <mark>ADD_MONTH</mark> function Implement ADD_MONTH function Aug 21, 2023
ever0de
ever0de reviewed Aug 23, 2023
View reviewed changes
Copy link
Member

@ever0de ever0de left a comment

Choose a reason for hiding this comment

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

I'd like to request a few minor changes

core/src/data/value/error.rs Outdated Show resolved Hide resolved
core/src/executor/evaluate/function.rs Outdated Show resolved Hide resolved
core/src/executor/evaluate/function.rs Outdated Show resolved Hide resolved
kite707 and others added 4 commits August 24, 2023 12:56
@kite707 @ever0de
Update core/src/data/value/error.rs
7cc2f0f 
Co-authored-by: Jiseok CHOI <jiseok.dev@gmail.com>
@kite707 @ever0de
Update core/src/executor/evaluate/function.rs
d8232f7 
Co-authored-by: Jiseok CHOI <jiseok.dev@gmail.com>
@kite707 @ever0de
Update core/src/executor/evaluate/function.rs
6a44149 
Co-authored-by: Jiseok CHOI <jiseok.dev@gmail.com>
@kite707
refactor: change error name
c093917
@kite707
Copy link
Contributor Author

kite707 commented Aug 24, 2023

Thank you for your advice! I have applied all the parts you advised!

@kite707 kite707 changed the title Implement ADD_MONTH function Implement ADD_MONTH function Aug 24, 2023
ever0de
ever0de approved these changes Aug 28, 2023
View reviewed changes
Copy link
Member

@ever0de ever0de left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. It looks great, but I can't get coverage-ci to run (rust version below 1.70), so if there's a rust version bump in the main in the near future, can you give the github action another try?

kite707 and others added 19 commits August 29, 2023 22:12
@kite707
test: add general test case of add_month
fc867c5
@kite707
test: add minus test
343f69c
@kite707
test: add test case of the last day of February
60594aa
@kite707
test: add test case of the year change
ae45887
@kite707
test: simplify testcase
cdc476d
@kite707
feat: develop add_month
6b15970
@kite707
feat: define ValueTou32ConversionFailure error
99575f9
@kite707
feat: develop sub month
bbda253
@kite707
test: add test case
e76ed49
@kite707
fix: fix test suite
8e9d761
@kite707
refactor: simplify add month code
9f85094
@kite707 @ever0de
Update core/src/data/value/error.rs
cd3a01c 
Co-authored-by: Jiseok CHOI <jiseok.dev@gmail.com>
@kite707 @ever0de
Update core/src/executor/evaluate/function.rs
e5bc484 
Co-authored-by: Jiseok CHOI <jiseok.dev@gmail.com>
@kite707 @ever0de
Update core/src/executor/evaluate/function.rs
00c1176 
Co-authored-by: Jiseok CHOI <jiseok.dev@gmail.com>
@kite707
refactor: change error name
3d33e0e
@kite707
feat: addMonth추가
8cc02a2
@kite707
Merge pull request #1 from kite707/copy_imple_add_date
991a95d 
Copy imple add date
@kite707
feat: addMonth삭제
5c0825a
@kite707
Merge branch 'gluesql:main' into imple_add_date
36d0669
panarch
panarch requested changes Sep 2, 2023
View reviewed changes
core/src/executor/evaluate/function.rs Outdated Show resolved Hide resolved
test-suite/src/function/add_month.rs Outdated Show resolved Hide resolved
@kite707
Copy link
Contributor Author

kite707 commented Sep 2, 2023

Thank you for your advice!

@panarch panarch self-requested a review September 2, 2023 05:43
panarch
panarch approved these changes Sep 2, 2023
View reviewed changes
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Looks all nice!
Thanks a lot for the contribution 👍 👍

@panarch panarch merged commit 583008b into gluesql:main Sep 2, 2023
9 checks passed
@panarch panarch mentioned this pull request May 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panarch panarch panarch approved these changes

@ever0de ever0de ever0de approved these changes

@devgony devgony Awaiting requested review from devgony

Assignees

@kite707 kite707

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement ADD_MONTH function
3 participants
@kite707 @panarch @ever0de