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

Implementation UTC_DATE/UTC_TIME/UTC_TIMESTAMP #127

Merged
merged 15 commits into from
Oct 17, 2022
Merged

Implementation UTC_DATE/UTC_TIME/UTC_TIMESTAMP #127

merged 15 commits into from
Oct 17, 2022

Conversation

MitchellGale
Copy link

Description

UTC_DATE, UTC_TIME, UTC_TIMESTAMP are all implemented according to MySQL standard.

Issues Resolved

Support date and time functions for new engine · Issue #709 · opendistro-for-elasticsearch/sql

Date time query improvement · Issue #46 · opensearch-project/sql

opensearchsql> select utc_date();                                                                                                                                                  
fetched rows / total rows = 1/1
+--------------+
| utc_date()   |
|--------------|
| 2022-10-03   |
+--------------+
opensearchsql> select utc_time();                                                                                                                                                  
fetched rows / total rows = 1/1
+--------------+
| utc_time()   |
|--------------|
| 17:54:27     |
+--------------+
opensearchsql> select utc_timestamp();                                                                                                                                             
fetched rows / total rows = 1/1
+---------------------+
| utc_timestamp()     |
|---------------------|
| 2022-10-03 17:54:28 |
+---------------------+

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 3, 2022
View reviewed changes
common/src/main/java/org/opensearch/sql/common/grok/GrokUtils.java Outdated
Comment on lines 27 to 38
"%\\{"
+ "(?<name>"
+ "(?<pattern>[A-z0-9]+)"
+ "(?::(?<subname>[A-z0-9_:;,\\-\\/\\s\\.']+))?"
+ ")"
+ "(?:=(?<definition>"
+ "(?:"
+ "(?:[^{}]+|\\.+)+"
+ ")+"
+ ")"
+ ")?"
+ "\\}");

Check failure

Code scanning / CodeQL

Inefficient regular expression

This part of the regular expression may cause exponential backtracking on strings starting with '%{{0=' and containing many repetitions of 'z'.
common/src/main/java/org/opensearch/sql/common/grok/GrokCompiler.java Outdated
public class GrokCompiler implements Serializable {

// We don't want \n and commented line
private static final Pattern patternLinePattern = Pattern.compile("^([A-z0-9_]+)\\s+(.*)$");

Check warning

Code scanning / CodeQL

Overly permissive regular expression range

Suspicious character range that is equivalent to \[A-Z\\[\\\\]^_`a-z\].
Copy link

Choose a reason for hiding this comment

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

@MitchellGale-BitQuill I think this was fixed in upstream -- if you update the integ branch, the warnings will go away.

@codecov
Copy link

codecov bot commented Oct 3, 2022 •
edited
Loading

Codecov Report

Merging #127 (90f4358) into integ-UTC_DateTime_functions (057fa44) will decrease coverage by 2.84%.
The diff coverage is 100.00%.

@@                        Coverage Diff                         @@
##             integ-UTC_DateTime_functions     #127      +/-   ##
==================================================================
- Coverage                           97.87%   95.02%   -2.85%     
- Complexity                           3020     3029       +9     
==================================================================
  Files                                 284      294      +10     
  Lines                                7425     8102     +677     
  Branches                              475      594     +119     
==================================================================
+ Hits                                 7267     7699     +432     
- Misses                                157      349     +192     
- Partials                                1       54      +53
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine 97.87% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/main/java/org/opensearch/sql/expression/DSL.java 100.00% <100.00%> (ø)
...arch/sql/expression/datetime/DateTimeFunction.java 100.00% <100.00%> (ø)
...h/sql/expression/function/BuiltinFunctionName.java 100.00% <100.00%> (ø)
workbench/public/components/Header/Header.tsx 100.00% <0.00%> (ø)
...h/public/components/QueryLanguageSwitch/Switch.tsx 85.71% <0.00%> (ø)
workbench/public/application.tsx 0.00% <0.00%> (ø)
workbench/public/components/SQLPage/SQLPage.tsx 100.00% <0.00%> (ø)
workbench/public/components/Main/main.tsx 53.00% <0.00%> (ø)
workbench/public/components/app.tsx 0.00% <0.00%> (ø)
...ch/public/components/QueryResults/QueryResults.tsx 61.60% <0.00%> (ø)
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

raymond-lum
raymond-lum reviewed Oct 3, 2022
View reviewed changes
core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java
/**
* To_days implementation for ExprValue.
*
* @return ExprValue.
Copy link

Choose a reason for hiding this comment

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

JavaDoc should explain what the return is.

Copy link
Author

Choose a reason for hiding this comment

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

@Yury-Fridlyand any advice on how to address this if not including "yyyy-MM-dd"

Copy link

Choose a reason for hiding this comment

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

You can mention return value type.

raymond-lum
raymond-lum reviewed Oct 3, 2022
View reviewed changes
Copy link

@raymond-lum raymond-lum left a comment •
edited
Loading

Choose a reason for hiding this comment

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

.

acarbonetto
acarbonetto requested changes Oct 4, 2022
View reviewed changes
core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated Show resolved Hide resolved
docs/user/dql/functions.rst Outdated Show resolved Hide resolved
docs/user/dql/functions.rst Outdated Show resolved Hide resolved
docs/user/ppl/functions/datetime.rst Outdated Show resolved Hide resolved
core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java Outdated
ExprStringValue localDateValue = new ExprStringValue(formattedLDT);
ExprStringValue toTZ = new ExprStringValue("+00:00");

return exprDateTime(localDateValue, toTZ);
Copy link

Choose a reason for hiding this comment

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

Is this supposed to return a Datetime or Timestamp?

Copy link
Author

Choose a reason for hiding this comment

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

Timestamp is formatted as a datetime. We have no exprTimeStamp.

Copy link

Choose a reason for hiding this comment

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

Would

opensearch-project-sql/core/src/main/java/org/opensearch/sql/expression/datetime/DateTimeFunction.java

Line 987 in bc91c66

private ExprValue exprTimestamp(ExprValue exprValue) {
work?

Copy link
Author

Choose a reason for hiding this comment

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

exprDateTime takes an argument of toTz which converts the localDateValue passed into UTC. exprTimestamp
does not have that implementation

docs/user/dql/functions.rst Show resolved Hide resolved
ppl/src/main/antlr/OpenSearchPPLParser.g4 Outdated Show resolved Hide resolved
sql/src/main/antlr/OpenSearchSQLParser.g4 Outdated Show resolved Hide resolved
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Oct 5, 2022
Open
MitchellGale
MitchellGale commented Oct 12, 2022
View reviewed changes
Copy link
Author

@MitchellGale MitchellGale left a comment •
edited
Loading

Choose a reason for hiding this comment

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

.

MitchellGale and others added 15 commits October 12, 2022 13:11
@MitchellGale
Added UTC_DATE, UTC_TIME, UTC_TIMESTAMP implementations.
5107309 
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Added doc tests and fixed java doc description for UTC TIME/DATE/TIME…
cc2d186 
…STAMP

Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Fixed some javadocs, and addressed other PR comments.
cfbfdf2 
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Fixed duplicate UTC functions in parser. Moved tests over to likenow …
7efe2e2 
…test suite.

Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Fixed UTC IT test issues with not using UTC time for functions.
fbf30d5 
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Switched timestamp return type to be DATETIME not TIMESTAMP.
02a1b40 
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
ADDED IT TESTS TO SQL
d9964c8 
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale @Yury-Fridlyand
Update docs/user/ppl/functions/datetime.rst
9c3f3a3 
Co-authored-by: Yury-Fridlyand <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale @Yury-Fridlyand
Update docs/user/ppl/functions/datetime.rst
1e85d3b 
Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/dql/functions.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/ppl/functions/datetime.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/dql/functions.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/dql/functions.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/ppl/functions/datetime.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/ppl/functions/datetime.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/ppl/functions/datetime.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/dql/functions.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/dql/functions.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update core/src/test/java/org/opensearch/sql/expression/datetime/NowLikeFunctionTest.java

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/dql/functions.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/ppl/functions/datetime.rst

Co-authored-by: Yury-Fridlyand <[email protected]>

Update docs/user/dql/functions.rst

Co-authored-by: Yury-Fridlyand <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Update core/src/test/java/org/opensearch/sql/expression/datetime/NowL…
648744c 
…ikeFunctionTest.java

Co-authored-by: Max Ksyunz <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Update integ-test/src/test/java/org/opensearch/sql/ppl/DateTimeFuncti…
50fdcf3 
…onIT.java

Co-authored-by: Max Ksyunz <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Update core/src/main/java/org/opensearch/sql/expression/datetime/Date…
2a23bf8 
…TimeFunction.java

Co-authored-by: Max Ksyunz <[email protected]>
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
resolved conflict.
d574625 
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Removed formatter
4f321a1 
Signed-off-by: MitchellGale-BitQuill <[email protected]>
@MitchellGale
Made PR requested changes
90f4358 
Signed-off-by: MitchellGale-BitQuill <[email protected]>
forestmvey
forestmvey reviewed Oct 13, 2022
View reviewed changes
core/src/test/java/org/opensearch/sql/expression/datetime/NowLikeFunctionTest.java
ExprCoreType resType,
Boolean hasFsp,
Supplier<Temporal> referenceGetter) {
@SuppressWarnings("unused") // Used in the test name above
Copy link

Choose a reason for hiding this comment

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

Was this indentation change needed?

@MitchellGale MitchellGale merged commit 4bd8d22 into integ-UTC_DateTime_functions Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raymond-lum raymond-lum raymond-lum left review comments

@forestmvey forestmvey forestmvey approved these changes

@Yury-Fridlyand Yury-Fridlyand Yury-Fridlyand approved these changes

@GabeFernandez310 GabeFernandez310 Awaiting requested review from GabeFernandez310

@GumpacG GumpacG Awaiting requested review from GumpacG

@acarbonetto acarbonetto Awaiting requested review from acarbonetto

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MitchellGale @Yury-Fridlyand @forestmvey @raymond-lum @MaxKsyunz @acarbonetto