Skip to content

new feature: allows user to specify timeZone in TimestampSpec#6365

Closed
zhaojiandong wants to merge 7 commits intoapache:masterfrom
zhaojiandong:featues/add-timeZone-to-timestampSpec
Closed

new feature: allows user to specify timeZone in TimestampSpec#6365
zhaojiandong wants to merge 7 commits intoapache:masterfrom
zhaojiandong:featues/add-timeZone-to-timestampSpec

Conversation

@zhaojiandong
Copy link
Copy Markdown
Contributor

@zhaojiandong zhaojiandong commented Sep 21, 2018 •
edited
Loading

new feature: allows user to specify timeZone in TimestampSpec, How to use:

"timestampSpec": {
  "format": "yyyy-MM-dd HH:mm:ss",
  "column": "logTime",
  "timeZone": "Asia/Shanghai"
}

The default value of timeZone is 'UTC'

@zhaojiandong
Copy link
Copy Markdown
Contributor Author

zhaojiandong commented Sep 22, 2018

Adding parameter 'timeZone' to TimestampSpec changes constructor signature, So this pr has involved many test files @gianm

@gianm gianm self-assigned this Sep 28, 2018
@gianm gianm added this to the 0.13.1 milestone Sep 28, 2018
@zhaojiandong zhaojiandong force-pushed the featues/add-timeZone-to-timestampSpec branch from 3dd08a6 to 614fac6 Compare January 18, 2019 07:52
@zhaojiandong
Copy link
Copy Markdown
Contributor Author

zhaojiandong commented Jan 18, 2019

Hi @gianm @asdf2014 , can you review my pr. This pr adds a field(timezone) to the constructor of TimestampSpec, so many classes have been modified.

@gianm gianm assigned clintropolis and unassigned gianm Feb 5, 2019
@gianm gianm removed this from the 0.14.0 milestone Feb 5, 2019
clintropolis
clintropolis reviewed Feb 6, 2019
View reviewed changes
Comment thread core/src/main/java/org/apache/druid/java/util/common/parsers/TimestampParser.java Outdated
Comment thread core/src/main/java/org/apache/druid/data/input/impl/TimestampSpec.java Outdated
Comment thread benchmarks/src/main/java/org/apache/druid/benchmark/FlattenJSONBenchmarkUtil.java Outdated
Comment thread docs/content/ingestion/ingestion-spec.md Outdated
@clintropolis
Copy link
Copy Markdown
Member

clintropolis commented Feb 6, 2019

Very sorry for the delayed review, thanks for your patience!

@clintropolis
Copy link
Copy Markdown
Member

clintropolis commented Feb 6, 2019

related #6343

zhaojiandong added 2 commits February 15, 2019 00:16
Squashed commit of the following:
4db2283 
commit f4d29ce2be6d83f77b6fbc4039276a57825cc4bb
Author: zhaojiandong <zhaojiandong@youzan.com>
Date:   Fri Jan 18 15:37:32 2019 +0800

    allow user to specify time at ingestion time

commit 28a060bf5fd0b4e33fbcca57cb819cd416490802
Author: zhaojiandong <zhaojiandong@youzan.com>
Date:   Fri Jan 18 13:33:11 2019 +0800

    allow user to specify time at ingestion time
add a doc
4ab1a76
@zhaojiandong
Copy link
Copy Markdown
Contributor Author

zhaojiandong commented Feb 14, 2019

Very sorry for the delayed review, thanks for your patience!

Thank you for your review 👍 @clintropolis

@zhaojiandong zhaojiandong force-pushed the featues/add-timeZone-to-timestampSpec branch from 09b52c8 to 0c0ba3a Compare February 15, 2019 02:49
clintropolis
clintropolis reviewed Feb 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

Overall LGTM 👍

Comment thread core/src/main/java/org/apache/druid/data/input/impl/TimestampSpec.java
clintropolis
clintropolis approved these changes Feb 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Feb 19, 2019

I restarted the TeamCity build; the errors it noticed were in unchanged files. If it trips again, @zhaojiandong, you might need to merge from master to get it to pass.

@zhaojiandong
Copy link
Copy Markdown
Contributor Author

zhaojiandong commented Feb 20, 2019

I've merged remote/upsteam/master to my branch and triggered the TeamCity build @gianm

@glasser
Copy link
Copy Markdown
Contributor

glasser commented Feb 21, 2019

FWIW my PR #7089 is also seeing 2800+ inspections in random files — so it doesn't seem specific to either of our PRs...

@clintropolis
Copy link
Copy Markdown
Member

clintropolis commented Feb 21, 2019

FWIW my PR #7089 is also seeing 2800+ inspections in random files — so it doesn't seem specific to either of our PRs...

Yeah, it seems to be an intermittent teamcity issue, unsure what causes it yet.

@zhaojiandong
Copy link
Copy Markdown
Contributor Author

zhaojiandong commented Feb 21, 2019

TeamCity build log, it looks like from a test method: testExtractTimestampWithTimezone

core/src/test/java/org/apache/druid/data/input/impl
TimestampSpecTest.java (1)
89: testExtractTimestampWithTimezone() Explicit type arguments can be inferred

@stale
Copy link
Copy Markdown

stale Bot commented Apr 22, 2019

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

@stale stale Bot added the stale label Apr 22, 2019
@clintropolis
Copy link
Copy Markdown
Member

clintropolis commented Apr 22, 2019

Apologies, this PR fell off my radar and I forgot about it, could you resolve the conflicts?

@stale stale Bot removed the stale label Apr 22, 2019
@vogievetsky
Copy link
Copy Markdown
Contributor

vogievetsky commented Jun 9, 2019

@zhaojiandong could you have a look at the conflicted files please?

@jihoonson
Copy link
Copy Markdown
Contributor

jihoonson commented Mar 25, 2020

@zhaojiandong apologize for delayed review.. Probably we should have merged your PR before. Would you please fix the conflicts? Then, I will merge this PR.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 16, 2023

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

@github-actions github-actions Bot added the stale label Jul 16, 2023
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 14, 2023

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

@github-actions github-actions Bot closed this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clintropolis clintropolis clintropolis approved these changes

Assignees

@clintropolis clintropolis

Labels

Feature stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@zhaojiandong @clintropolis @gianm @glasser @vogievetsky @jihoonson