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.

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

[CALCITE-6241] Enable a few existing functions to hive library #3948

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ChengJie1053
Copy link
Member

@ChengJie1053 ChengJie1053 commented Sep 4, 2024

Add hive as a supported library for functions that have already been implemented for other libraries.

Hive Functions Link:https://cwiki.apache.org/confluence/display/Hive/LanguageManual+UDF#LanguageManualUDF-ArithmeticOperators

@caicancai
Copy link
Member

caicancai commented Sep 4, 2024

@ChengJie1053 Thanks for your contribution, can you confirm that these functions give the same results in some boundary tests?

In my experience, this is difficult to guarantee.

Copy link
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

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

I haven't verified that indeed all these functions in Hive have the exact same behavior as the existing implementation. But assuming they do, this PR is fine.

@ChengJie1053
Copy link
Member Author

I haven't verified that indeed all these functions in Hive have the exact same behavior as the existing implementation. But assuming they do, this PR is fine.

Ok, thanks for reviewing the code for me

@ChengJie1053
Copy link
Member Author

@ChengJie1053 Thanks for your contribution, can you confirm that these functions give the same results in some boundary tests?

In my experience, this is difficult to guarantee.

Ok, thanks for reviewing the code for me, are there any other test cases I need to modify for testing

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.

Please remove the adaptation of log related functions. I have not adapted the log function to hive.

@ChengJie1053
Copy link
Member Author

Please remove the adaptation of log related functions. I have not adapted the log function to hive.

Ok, thanks for reviewing the code for me, I will change the code

Copy link
Contributor

@NobiGo NobiGo left a comment

Choose a reason for hiding this comment

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

LGTM. If we discover inconsistent function semantics, we can resolve it by a new ISSUE.

@mihaibudiu mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 5, 2024
@ChengJie1053
Copy link
Member Author

LGTM. If we discover inconsistent function semantics, we can resolve it by a new ISSUE.

Ok, thanks for reviewing the code for me

@caicancai
Copy link
Member

Please remove the log function before merging.

@ChengJie1053
Copy link
Member Author

Please remove the log function before merging.

Ok, thanks for reviewing the code for me, I will change the code

@ChengJie1053 ChengJie1053 force-pushed the main-SqlLibraryOperators branch 2 times, most recently from 6f1e4a5 to 3ef2028 Compare September 6, 2024 02:00
@ChengJie1053
Copy link
Member Author

Please remove the log function before merging.

Hello, the code has been modified

@mihaibudiu
Copy link
Contributor

Can you please fix the conflicts and squash the commits in preparation for merging?

@caicancai
Copy link
Member

@ChengJie1053

Can you please fix the conflicts and squash the commits in preparation for merging?

@ChengJie1053
Copy link
Member Author

Can you please fix the conflicts and squash the commits in preparation for merging?

Ok, thank you. I'll fix it as soon as possible

@mihaibudiu
Copy link
Contributor

The 1.38 release is coming soon, if you can squash the commits we can merge this.

@ChengJie1053
Copy link
Member Author

The 1.38 release is coming soon, if you can squash the commits we can merge this.

Ok, thank you. I tried squash commits, but failed. Can I submit a new pr

@mihaibudiu
Copy link
Contributor

There is no reason squashing cannot be done. Try git reset HEAD~5 and then you can start as if there is no commit at all. You just have to use -f when you push again

@ChengJie1053
Copy link
Member Author

There is no reason squashing cannot be done. Try git reset HEAD~5 and then you can start as if there is no commit at all. You just have to use -f when you push again

Ok, thank you very much. I'll try that

@ChengJie1053 ChengJie1053 force-pushed the main-SqlLibraryOperators branch 6 times, most recently from 66a74bb to 5384115 Compare September 20, 2024 09:17
Copy link

sonarcloud bot commented Sep 20, 2024

@mihaibudiu
Copy link
Contributor

This PR is fine, but the JIRA issue CALCITE-6241 has a different title and has been closed for 1.37.
There are also conflicts.
One course of action is to remove the CALCITE-6241 from the PR and commit message.
Another course of action is to file a new JIRA issue and use that.

@mihaibudiu
Copy link
Contributor

Unfortunately this has developed new conflicts, can you please solve these and we can now merge it.

@mihaibudiu
Copy link
Contributor

Tried to fix the conflict using the browser editor and I failed... sorry about this.

@mihaibudiu
Copy link
Contributor

@ChengJie1053 if you can fix the conflicts we can hopefully merge this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants