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

Added support for STRRIGHT alias in Doris (#31508) #33393

Merged
merged 6 commits into from
Oct 27, 2024

Conversation

danigiri
Copy link
Contributor

@danigiri danigiri commented Oct 24, 2024

Continuing with Doris parsing support.
In this case RIGHT is already implemented and supported by both MySQL and Doris so ammended the tests and added STRRIGHT support which is an alias in Doris.

Continues to address #31508.

Changes proposed in this pull request:

  • Added parsing support for the STRRIGHT alias in Doris
  • Updated existing MySQL test to reflect that RIGHT is also supported by Doris
  • Added STRRIGHT-specific test
  • Marked // DORIS ADDED BEGIN|END Doris-specific changes, and fixed a typo in existing one
  • Added as a regularFunctionName which is where RIGHT is so no java changes should be needed
  • Executed SELECT STRRIGHT('foobarbar',4) in Doris 3.0.2 correctly

Next

  • get PR id to update release notes
  • finish compiling Doris v3 to test the query code my local machine is not powerful enough

Before committing this PR, I'm sure that I have checked the following options:

  • [✅] My code follows the code of conduct of this project.
  • [✅] I have self-reviewed the commit code.
  • [✅] I have (or in comment I request) added corresponding labels for the pull request.
  • [✅] I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • [✅] I have made corresponding changes to the documentation.
  • [✅] I have added corresponding unit tests for my changes.
  • [✅] mvn spotless:apply -Pcheck
  • [✅] mvn test -Dit.test=InternalDorisParserIT in shardingsphere/parser/sql/dialect/doris (need to specify it.test)
  • [✅] Executed test case in Doris v2.0.3 – SELECT STRRIGHT('foobarbar',4)

RIGHT is already implemented and supported by both MySQL and Doris
- Added parsing suppor t for the `STRRIGHT` alias in Doris
- Updated existing tests to reflect that `RIGHT` is also supported by Doris
- Added STRRIGHT tests
- Marked `// DORIS ADDED BEGIN|END` Doris-specific changes
- Added as a `regularFunctionName` which is where `RIGHT` is
@danigiri danigiri marked this pull request as ready for review October 24, 2024 10:24
@danigiri danigiri changed the title Added support for STRRIGTH alias in Doris (#31508) Added support for STRRIGHT alias in Doris (#31508) Oct 24, 2024
@danigiri
Copy link
Contributor Author

I will look at the test results again as 76b9007 was supposed to fix the failing test

also removed MySQL as target for the alias
@danigiri
Copy link
Contributor Author

Looks like on the most current code mvn test -Dtest=InternalDorisParserIT is no longer picking up the tests, using mvn test -Dit.test=InternalDorisParserIT does not give an error but looks like it does not pick it up either.
The most consistent way for me to test is to do a full rebuild in one of my separate test machines after a commit but that is a bit slow.

Waiting for that full rebuild to finish and issuing a fresh commit

@danigiri
Copy link
Contributor Author

All checks passed and it looks good, just did a merge with upstream to have it up to date. Checks will be redone but not expecting any issues there. Ready to be reviewed

Copy link
Member

@strongduanmu strongduanmu left a comment

Choose a reason for hiding this comment

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

Looks great, merged. @danigiri Thank you for your excellent code.

@strongduanmu strongduanmu merged commit e2f9597 into apache:master Oct 27, 2024
141 checks passed
@danigiri danigiri deleted the dev-doris branch November 4, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants