Skip to content

[CALCITE-5807] Add SUBSTRING_INDEX function (enabled in Spark library)#3292

Merged
mihaibudiu merged 1 commit intoapache:mainfrom
Rocky0714:CALCITE-5807
Sep 9, 2024
Merged

[CALCITE-5807] Add SUBSTRING_INDEX function (enabled in Spark library)#3292
mihaibudiu merged 1 commit intoapache:mainfrom
Rocky0714:CALCITE-5807

Conversation

@Rocky0714
Copy link
Contributor

No description provided.

@Rocky0714 Rocky0714 changed the title CALCITE-5807 Add SUBSTRING_INDEX function (enabled in Spark library) [CALCITE-5807] Add SUBSTRING_INDEX function (enabled in Spark library) Jul 4, 2023
@Rocky0714 Rocky0714 marked this pull request as draft July 4, 2023 06:40
@Rocky0714 Rocky0714 marked this pull request as ready for review July 6, 2023 13:04
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.

Left some comments. Please take a look when you are free.

@ILuffZhe ILuffZhe self-assigned this Jul 11, 2023
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.

Hi @hujianhong , left some comments.
One small tip: before the final squash and rebase, please don't force push your commit. It helps other reviewers to see the differences.

@Rocky0714
Copy link
Contributor Author

Hi @hujianhong , left some comments. One small tip: before the final squash and rebase, please don't force push your commit. It helps other reviewers to see the differences.

I'm sorry. This is my first PR for calcite. I will be careful in the future.

@ILuffZhe
Copy link
Contributor

There are some specific rules for commit message after c0e6ba2. Could you please amend yours so I can approve the following CI?

[invalid git log message 'fix and add test cases'; Message must start with upper-case letter]

@Rocky0714
Copy link
Contributor Author

There are some specific rules for commit message after c0e6ba2. Could you please amend yours so I can approve the following CI?

[invalid git log message 'fix and add test cases'; Message must start with upper-case letter]

done

@Rocky0714
Copy link
Contributor Author

@ILuffZhe Can you help me find out what is the reason of the CI task failed? The CI log information seem not match to my PR.
image

@NobiGo
Copy link
Contributor

NobiGo commented Jul 21, 2023

@hujianhong The CI task failure was caused by another PR and has been resolved. So don't worry.

@ILuffZhe
Copy link
Contributor

@hujianhong You can refer to the Calcite Website(https://calcite.apache.org/develop/#contributing).
In the special case, that the CI build failed, and the failure is not caused by your changes create an empty commit (git commit --allow-empty) and push it.

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.

LGTM! Let's see if there are any new comments come.

@ILuffZhe ILuffZhe added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 26, 2023
@ILuffZhe
Copy link
Contributor

Hi @hujianhong , could you please rebase and squash your commits? I'll merge this in the next few days.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

93.2% 93.2% Coverage
0.0% 0.0% Duplication

@mihaibudiu
Copy link
Contributor

@hujianhong if you can squash the two commits we can merge this PR. Thank you.

@mihaibudiu
Copy link
Contributor

Can you please rebase and squash the commits so we can merge this PR?

@Rocky0714
Copy link
Contributor Author

Can you please rebase and squash the commits so we can merge this PR?

Ok, I will rebase and squash.

@mihaibudiu
Copy link
Contributor

The CI is unhappy, but I think there are only small changes necessary to make it pass.
If you do them, you can probably squash the commits directly.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 8, 2024

@Rocky0714
Copy link
Contributor Author

The CI is unhappy, but I think there are only small changes necessary to make it pass. If you do them, you can probably squash the commits directly.

Done, thanks.

@mihaibudiu mihaibudiu merged commit 9723741 into apache:main Sep 9, 2024
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.

5 participants