Skip to content

[CALCITE-6223] Add MAP_CONTAINS_KEY function (enabled in Spark library)#3655

Merged
macroguo-ghy merged 1 commit intoapache:mainfrom
caicancai:6223
Feb 7, 2024
Merged

[CALCITE-6223] Add MAP_CONTAINS_KEY function (enabled in Spark library)#3655
macroguo-ghy merged 1 commit intoapache:mainfrom
caicancai:6223

Conversation

@caicancai
Copy link
Member

@caicancai caicancai commented Jan 27, 2024

@caicancai
Copy link
Member Author

caicancai commented Jan 27, 2024

@mihaibudiu @macroguo-ghy Sorry, there was something wrong with the branch management before, so I submitted a PR again.

Copy link
Contributor

@macroguo-ghy macroguo-ghy left a comment

Choose a reason for hiding this comment

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

Left some comments

@caicancai caicancai force-pushed the 6223 branch 2 times, most recently from b7daeac to f6d6830 Compare January 29, 2024 04:52
@caicancai caicancai requested a review from mihaibudiu January 29, 2024 13:52
@caicancai caicancai force-pushed the 6223 branch 2 times, most recently from a1796b7 to 16fd010 Compare January 30, 2024 09:47
@caicancai caicancai requested a review from mihaibudiu January 30, 2024 09:48
@caicancai caicancai force-pushed the 6223 branch 2 times, most recently from 4da1807 to defabce Compare February 4, 2024 08:07
@caicancai caicancai requested a review from tanclary February 4, 2024 09:58
@caicancai
Copy link
Member Author

@macroguo-ghy Would you mind helping me take a look, thank you

Copy link
Contributor

@macroguo-ghy macroguo-ghy left a comment

Choose a reason for hiding this comment

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

I think as a committer of an Apache project, you must be an excellent engineer, but in this PR, you have made too many low-level errors, and even copied a lot of code from other places. I hope you can double check your code before submitting the PR.

I'm sorry for being a bit harsh, but I hope to save time for all contributors (including reviewers) in the Calcite community, as most of us participate in community activities during our personal time.

Another thing, try not to use rebase or square unless requested by the committer, otherwise reviewers can not check the diff between commits.

In order to update the pull request, you need to commit the changes in your branch and then push the commit(s) to GitHub. You are encouraged to use regular (non-rebased) commits on top of previously existing ones.
When pushing the changes to GitHub, you should refrain from using the --force parameter and its alternatives. You may choose to force push your changes under certain conditions:
the pull request has been submitted less than 10 minutes ago and there is no pending discussion (in the PR and/or in JIRA) concerning it;
a reviewer has explicitly asked you to perform some modifications that require the use of the --force option.

See details in develop guide.

@caicancai
Copy link
Member Author

caicancai commented Feb 5, 2024

I think as a committer of an Apache project, you must be an excellent engineer, but in this PR, you have made too many low-level errors, and even copied a lot of code from other places. I hope you can double check your code before submitting the PR.

I'm sorry for being a bit harsh, but I hope to save time for all contributors (including reviewers) in the Calcite community, as most of us participate in community activities during our personal time.

Another thing, try not to use rebase or square unless requested by the committer, otherwise reviewers can not check the diff between commits.

In order to update the pull request, you need to commit the changes in your branch and then push the commit(s) to GitHub. You are encouraged to use regular (non-rebased) commits on top of previously existing ones.
When pushing the changes to GitHub, you should refrain from using the --force parameter and its alternatives. You may choose to force push your changes under certain conditions:
the pull request has been submitted less than 10 minutes ago and there is no pending discussion (in the PR and/or in JIRA) concerning it;
a reviewer has explicitly asked you to perform some modifications that require the use of the --force option.

See details in develop guide.

Thank you very much for your suggestions. Recently, the PRs of my two adaptation functions have caused a lot of trouble for everyone. In other words, some of my recent PRs have more or less made some low-level mistakes. I am also reflecting on myself and causing you problems. I'm sorry for bothering you. I won't make similar low-level mistakes again in the future.

Copy link
Contributor

@macroguo-ghy macroguo-ghy left a comment

Choose a reason for hiding this comment

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

+1

Please squash your commit, I will merge it later.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 6, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
90.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@caicancai
Copy link
Member Author

Thank you for your review

@macroguo-ghy macroguo-ghy merged commit cf91b78 into apache:main Feb 7, 2024
@caicancai caicancai deleted the 6223 branch February 7, 2024 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants