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

[CALCITE-6800] Enable a few existing functions to HIVE library #4165

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[CALCITE-6800] Enable a few existing functions to HIVE library #4165

wants to merge 1 commit into from

Conversation

xuzifu666
Copy link
Member

@xuzifu666 xuzifu666 commented Jan 24, 2025 •
edited
Loading

Enable some methods to hive which had already been implemented for other libraries.
issue: https://issues.apache.org/jira/browse/CALCITE-6800
support functions as follow:
DATE_ADD
DATE_SUB
DECODE
LPAD
RPAD
LTRIM
RTRIM
INSTR
GREATEST
LEAST
REGEXP_REPLACE_3
REPEAT
SOUNDEX
TO_DATE
MD5
SHA1
LOG_MYSQL

caicancai
caicancai approved these changes Jan 24, 2025
View reviewed changes
Copy link
Member

@caicancai caicancai left a comment

Choose a reason for hiding this comment

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

It seems that the bounds test of the function is not considered for the time being.

LGTM

@xuzifu666 xuzifu666 force-pushed the calcite-6241 branch 2 times, most recently from 79cfb54 to 440101a Compare January 24, 2025 06:14
@ILuffZhe
Copy link
Contributor

@xuzifu666 The CI failed ,please make sure your change can pass on your machine, then push it so we can go forward.

@xuzifu666 xuzifu666 force-pushed the calcite-6241 branch 3 times, most recently from f54e426 to dc885b1 Compare January 24, 2025 11:11
@xuzifu666 xuzifu666 closed this Jan 24, 2025
@xuzifu666 xuzifu666 reopened this Jan 24, 2025
@xuzifu666
Copy link
Member Author

@xuzifu666 The CI failed ,please make sure your change can pass on your machine, then push it so we can go forward.

@ILuffZhe CI is success, PTAL when you are free time, thanks~

ILuffZhe
ILuffZhe reviewed Jan 25, 2025
View reviewed changes
Copy link
Contributor

@ILuffZhe ILuffZhe left a comment

Choose a reason for hiding this comment

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

It's really close now, could you please take a look? @xuzifu666

site/_docs/reference.md Show resolved Hide resolved
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java Outdated Show resolved Hide resolved
@xuzifu666
Copy link
Member Author

@ILuffZhe addressed，thanks for your review，PTAL

ILuffZhe
ILuffZhe reviewed Jan 25, 2025
View reviewed changes
site/_docs/reference.md Outdated
@@ -2852,7 +2852,7 @@ In the following:
| b | DATE(string) | Equivalent to `CAST(string AS DATE)`
| b | DATE(year, month, day) | Returns a DATE value for *year*, *month*, and *day* (all of type INTEGER)
| q r f | DATEADD(timeUnit, integer, datetime) | Equivalent to `TIMESTAMPADD(timeUnit, integer, datetime)`
| q r f | DATEDIFF(timeUnit, datetime, datetime2) | Equivalent to `TIMESTAMPDIFF(timeUnit, datetime, datetime2)`
| q r f h | DATEDIFF(timeUnit, datetime, datetime2) | Equivalent to `TIMESTAMPDIFF(timeUnit, datetime, datetime2)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a unintentional mistake?

Copy link
Member Author

Choose a reason for hiding this comment

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

Had removed it.

@ILuffZhe
Copy link
Contributor

ILuffZhe commented Jan 25, 2025 •
edited
Loading

Hi @xuzifu666 , you don't need to force-push every time, which makes it not easier to review.
In this case, make sure all the functions enabled in Hive library is document in reference.md, and SqlOperatorTest should also cover those functions (the last commit still missed some tests).
Please take a look and cc me when you are ready.

By the way, I think it might be better to add a new JIRA ticket, CALCITE-6241 is for Spark library.

@ILuffZhe ILuffZhe self-assigned this Jan 25, 2025
@xuzifu666 xuzifu666 changed the title [CALCITE-6241] Enable a few existing functions to hive library [CALCITE-6800] Enable a few existing functions to hive library Jan 25, 2025
@xuzifu666
Copy link
Member Author

xuzifu666 commented Jan 25, 2025 •
edited
Loading

Hi @xuzifu666 , you don't need to force-push every time, which makes it not easier to review. In this case, make sure all the functions enabled in Hive library is document in reference.md, and SqlOperatorTest should also cover those functions (the last commit still missed some tests). Please take a look and cc me when you are ready.

By the way, I think it might be better to add a new JIRA ticket, CALCITE-6241 is for Spark library.

Very thanks for your remind, I had add all changes in doc and add all cases in Test, I also make a new jira https://issues.apache.org/jira/browse/CALCITE-6800 about the pr, due to had a keyword 'hive' in title which not pass for ut, so I had rebase for the error, PTAL when you free time. @ILuffZhe

@xuzifu666 xuzifu666 changed the title [CALCITE-6800] Enable a few existing functions to hive library [CALCITE-6800] Enable a few existing functions to HIVE library Jan 25, 2025
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
20.5% Duplication on New Code

See analysis details on SonarQube Cloud

@ILuffZhe
Copy link
Contributor

It seems your changes in PR is not consistent with what described in JIRA.

@xuzifu666
Copy link
Member Author

xuzifu666 commented Jan 26, 2025 •
edited
Loading

It seems your changes in PR is not consistent with what described in JIRA.

This pr is aim to enable some existing functions to Hive library, I had changed update jira for all functions. Would you please give a review again please. Thanks~ @ILuffZhe

@ILuffZhe
Copy link
Contributor

It seems your changes in PR is not consistent with what described in JIRA.

This pr is aim to enable some existing functions to Hive library, I had changed purpose about it. Would you give some advice if it is not fitable. Thanks~ @ILuffZhe

This case should only enable existing functions which have the same behavior in Hive.
If not, maybe we should file a new JIRA ticket and implement it if you want.

@xuzifu666
Copy link
Member Author

xuzifu666 commented Jan 26, 2025 •
edited
Loading

It seems your changes in PR is not consistent with what described in JIRA.

This pr is aim to enable some existing functions to Hive library, I had changed purpose about it. Would you give some advice if it is not fitable. Thanks~ @ILuffZhe

This case should only enable existing functions which have the same behavior in Hive.
If not, maybe we should file a new JIRA ticket and implement it if you want.

This case is only enable existing functions which have the same behavior in Hive，not implement for new functions，so keep it can be fine without file a new jira ticket or can merge this pr firstly? @ILuffZhe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ILuffZhe ILuffZhe ILuffZhe left review comments

@caicancai caicancai caicancai approved these changes

Assignees

@ILuffZhe ILuffZhe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xuzifu666 @ILuffZhe @caicancai