Add new functions to format datetime in joda datetime style #43818
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
taiyang-li
changed the title
|
zhanglistar
reviewed
Dec 2, 2022
|
The new adding function SELECT count(1)
FROM
(
SELECT fromUnixTimestampWithJodaPattern(1483200000 + number , 'yyyy-MM-dd HH:mm:ss')
FROM numbers(10000000)
)
Costs 1.6s
SELECT count(1)
FROM
(
SELECT FROM_UNIXTIME(1483200000 + id, 'yyyy-MM-dd HH:mm:ss')
FROM range(10000000)
)
Costs 4.482 s |
rschu1ze
reviewed
Dec 4, 2022
rschu1ze
reviewed
Dec 5, 2022
rschu1ze
reviewed
Dec 5, 2022
rschu1ze
reviewed
Dec 5, 2022
rschu1ze
reviewed
Dec 5, 2022
rschu1ze
reviewed
Dec 5, 2022
|
@rschu1ze Thanks very much for your review. Let's wait for the CI/CD pipelines. |
rschu1ze
reviewed
Dec 7, 2022
There was a problem hiding this comment.
@taiyang-li Thanks for your fixes. The PR looks overall good and is (almost) ready to merge. When I did the first round of review over the weekend, I skipped the tests (sorry). I now had a look at them, so please find some further comments included. It is mostly minor stuff, so we should be able to merge this PR afterwards.
tests/queries/0_stateless/02496_format_datetime_in_joda_syntax.reference
Outdated
Show resolved
Hide resolved
tests/queries/0_stateless/02496_format_datetime_in_joda_syntax.sql
Outdated
Show resolved
Hide resolved
alexey-milovidov
added
the
can be tested
|
LGTM. Style check (rightfully) complains that "formatDateTime.cpp" prints to stdout. Once you removed that (I don't have permissions to fix it by myself) and test are green, I will merge. Thanks! |
robot-ch-test-poll
added
the
submodule changed
rschu1ze
approved these changes
Dec 12, 2022
robot-ch-test-poll1
removed
the
submodule changed
|
@taiyang-li About the failure of fasttest, please see my earlier comment #43818 (comment). I also wonder why commit eb9d496 is needed? Submodules updated in other PRs are automatically obtained by merges from master. To get rid of this commit, you could squash your (and only your) commits, rebase them on top of the current master branch state and force-push to branch "improve_from_unixtime". |
taiyang-li
force-pushed
the
improve_from_unixtime
branch
from
eb9d496
to
58cbd81
Compare
|
@rschu1ze We'd better add label 'pr-performance'. Because this pr may affect function |
rschu1ze
added
the
pr-performance
robot-ch-test-poll
removed
the
pr-performance
|
Functional tests look good. Let me merge once all builds/tests are done (please don't merge from master in the meantime). |
|
The failed tests seem not related to this PR, see similar failed tests in #43239 |
|
Clang-tidy complains ... because of that the merge button is greyed out for me, sorry. |
|
The failed tests seem not related to this PR. |
|
Yes, the test failures are unrelated. However, #44060 was a little bit quicker to merge and created merge conflicts, sorry. Would you like to resolve these first? Thanks! |
|
Sorry, I had a fever in recent days. I will continue this work when I'm fine. |
15 tasks
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
format datetime in joda datetime style. Refer to https://joda-time.sourceforge.net/apidocs/org/joda/time/format/DateTimeFormat.html