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

Add zero period support to TIMESTAMPADD #10550

Merged
merged 4 commits into from
Nov 19, 2020
Merged

Add zero period support to TIMESTAMPADD #10550

merged 4 commits into from
Nov 19, 2020

Conversation

FrankChen021
Copy link
Member

Description

This PR solves #10530 by removing internal checking of zero period for TIMESTAMPADD SQL function and timestamp_shift native query.

As is described in the issue, TIMESTAMPADD uses shared code PeriodGranularity, which does not allow zero period for user configuration, to hold arguments passed to it. To fix the issue in a simple way, period and timezone are extracted from given arguments respectively instead of creating a PeriodGranularity object to hold them together.

And also, there's another slight improvement in code where TIMESTAMPADD SQL is converted to native query timestamp_shift function call. By detecting SQL AST ahead, a period string such as P1M is generated instead of calling of concat function.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@FrankChen021
Copy link
Member Author

@gianm please review this PR at your convenience. And CI reports lower branch coverage of TimestampShiftExprMacro in processing module, while this class has been fully covered by ExprMacroTest which is in server module. What should I do to the CI ?

@gianm
Copy link
Contributor

gianm commented Nov 4, 2020

And CI reports lower branch coverage of TimestampShiftExprMacro in processing module, while this class has been fully covered by ExprMacroTest which is in server module. What should I do to the CI ?

You could add a test similar to TimestampExtractExprMacroTest; it's in the same module, and it's a bit more unit-test-like than ExprMacroTest, so probably a good idea anyway.

@FrankChen021
Copy link
Member Author

Hi @gianm , now the CI reports some IT failures which I think are not related to the changes in this PR. I encounter same errors when running IT cases on master branch on my local env.

@zhangyue19921010
Copy link
Contributor

Maybe you can run the continuous-integration/travis-ci/pr — The Travis CI build failed again. It works for me to fix this kind of error.

gianm
gianm approved these changes Nov 13, 2020
View reviewed changes
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM after the tests pass. Thanks!

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
@@ -16016,6 +16017,142 @@ public void testTopNOnStringWithNonSortedOrUniqueDictionary(Map<String, Object>
);
}

@Test
public void testTimeStampAddZeroDayPeriod() throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tests!

@gianm
Copy link
Contributor

gianm commented Nov 13, 2020

@FrankChen021 I re-ran the tests that didn't pass. The failure might have been transient.

@gianm gianm closed this Nov 13, 2020
@gianm gianm reopened this Nov 13, 2020
@gianm
Copy link
Contributor

gianm commented Nov 13, 2020

Nevermind, the re-run itself failed. Very strange. I tried a close and reopen to see if that works.

@gianm
Copy link
Contributor

gianm commented Nov 16, 2020

@FrankChen021 Could you please merge master back into your branch? I'm wondering if that will help with the tests.

@FrankChen021
Copy link
Member Author

The branch has been rebased onto the master. Let's see if it helps

@FrankChen021
Copy link
Member Author

IT runs OK now, but the result of LGTM analysis is a little bit strange. I follow the details link, it shows all sub-steps are successful. I'll close and re-open to re-trigger the CI.

Pull request #10550 build logs—Succeeded
This page displays log information for the analysis of the code changes introduced by one commit.

Java—Succeededa day ago
Preparing merge commit (a82b749)—Succeeded
Build merge (7368a3c)—Succeeded (lines of code: 706.2k analyzed; 706.2k estimated)
Build master (3447934)—Succeeded (lines of code: 705.9k analyzed; 705.9k estimated)

@FrankChen021 FrankChen021 reopened this Nov 18, 2020
@suneet-s suneet-s merged commit d7d2c80 into apache:master Nov 19, 2020
@FrankChen021 FrankChen021 deleted the zero_period branch November 19, 2020 02:33
@jihoonson jihoonson mentioned this pull request Dec 9, 2020
Closed
@jihoonson jihoonson added the Bug label Dec 9, 2020
abhishekagarwal87 pushed a commit to abhishekagarwal87/druid that referenced this pull request Dec 14, 2020
@clintropolis @suneet-s
@capistrant @a2l007 @FrankChen021 @martin-g @vogievetsky @zhangyue19921010
ingest-service (#84)
5fbacec 
* scaffolding

* readme

* adjust

* more better, janky heap metadata store, primitive job queue that can submit to overlord, it works - sort of

* test scaffolding

* move InputFormat into IngestSchema

* imply-5135 Create & list ingest tables

* Addressed PR comments

* Removed bean IngestTable

* job processing + sql metadata job table (#78)

* Add indexed-table-loader (#65)

* Add indexed-table-loader

* Fix checkstyle

* Fix intelliJ inspections

* Fix analyze dependencies

* fix license check job

* Add imply-druid-security (#66)

* Add imply-druid-security

* fix checkstyle

* Fix analyze dependencies

* Fix license check job

* Update license header for all imply extensions

* fix intelliJ inspections

* code review

* modify access to protected SQLMetadataConnector methods to allow extensions to create SQL metadata tables using implementation specific constructs (payload type, serial type, etc) (apache#10573)

* Correct getRandomBalancerSegmentHolderTest (apache#10569)

* Add missing docs for timeout exceptions (apache#10554)

* Add missing docs for timeout exceptions

* Add info on auth failures

* Fix ingestion failure of pretty-formatted JSON message (apache#10383)

* support multi-line text

* add test cases

* split json text into lines case by case

* improve exception handle

* fix CI

* use IntermediateRowParsingReader as base of JsonReader

* update doc

* ignore the non-immutable field in test case

* add more test cases

* mark `lineSplittable` as final

* fix testcases

* fix doc

* add a test case for SqlReader

* return all raw columns when exception occurs

* fix CI

* fix test cases

* resolve review comments

* handle ParseException returned by index.add

* apply Iterables.getOnlyElement

* fix CI

* fix test cases

* improve code in more graceful way

* fix test cases

* fix test cases

* add a test case to check multiple json string in one text block

* fix inspection check

* Add TravisCI job that builds and tests on ARM64 CPU architecture (apache#10562)

* Ensure Krb auth before killing YARN apps in graceful shutdown (apache#9785)

* job processing + sql metadata

* Web console: fix data loader schema table column ordering bug and other polish (apache#10588)

* remove unused fields

* keep tables live

* advanced

* fix schema view

* better indication

* tests pass

* Show more instead of show advanced

* fix tests

* extract dynamic configs

* update snapshots

* fix issues

* update snapshot

* reword without >

* some javadoc

* modify druid.historical.cache.maxEntrySize property in Unified format (apache#10590)

Co-authored-by: yuezhang <yuezhang@freewheel.tv>

* Fix license header for imply extensions (#76)

* Fix license header for imply extensions

* arm64 packaging should use jdk8

* maybe this time

* jobs and states and status and whatever

* use indexing client and coordinator client instead of leader client

* always running

* simplify

* fix readme

* Add zero period support to TIMESTAMPADD (apache#10550)

* Allow zero period for TIMESTAMPADD

* update test cases

* add empty zone test case

* add unit test cases for TimestampShiftMacro

* add -Pimply-saas distribution profile, table exists check

* update readme

Co-authored-by: Suneet Saldanha <suneet.saldanha@imply.io>
Co-authored-by: Lucas Capistrant <capistrant@users.noreply.github.com>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Atul Mohan <atulmohan.mec@gmail.com>
Co-authored-by: frank chen <frank.chen021@outlook.com>
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Co-authored-by: Suneet Saldanha <suneet@apache.org>
Co-authored-by: Vadim Ogievetsky <vadim@ogievetsky.com>
Co-authored-by: zhangyue19921010 <69956021+zhangyue19921010@users.noreply.github.com>
Co-authored-by: yuezhang <yuezhang@freewheel.tv>

* fix style and headers

* fix fails

* fix auth

Co-authored-by: Agustin Gonzalez <agustin.gonzalez@imply.io>
Co-authored-by: Suneet Saldanha <suneet.saldanha@imply.io>
Co-authored-by: Lucas Capistrant <capistrant@users.noreply.github.com>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Atul Mohan <atulmohan.mec@gmail.com>
Co-authored-by: frank chen <frank.chen021@outlook.com>
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Co-authored-by: Suneet Saldanha <suneet@apache.org>
Co-authored-by: Vadim Ogievetsky <vadim@ogievetsky.com>
Co-authored-by: zhangyue19921010 <69956021+zhangyue19921010@users.noreply.github.com>
Co-authored-by: yuezhang <yuezhang@freewheel.tv>
@jihoonson jihoonson added this to the 0.21.0 milestone Jan 4, 2021
@jihoonson jihoonson mentioned this pull request Jan 13, 2021
Closed
JulianJaffePinterest pushed a commit to JulianJaffePinterest/druid that referenced this pull request Jan 22, 2021
@FrankChen021 @JulianJaffePinterest
Add zero period support to TIMESTAMPADD (apache#10550)
dbec0b7 
* Allow zero period for TIMESTAMPADD

* update test cases

* add empty zone test case

* add unit test cases for TimestampShiftMacro
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianm gianm gianm approved these changes

Assignees
No one assigned
Labels
Area - Querying Bug Feature/Change Description
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.
5 participants
@FrankChen021 @gianm @zhangyue19921010 @jihoonson @suneet-s