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 EXTRACT_URL_PARAMETER to Doris parsing support (#31508) #33571

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

danigiri
Copy link
Contributor

@danigiri danigiri commented Nov 6, 2024

Continuing with Doris parsing support.
In this case adding support for EXTRACT_URL_PARAMETER
see https://doris.apache.org/docs/sql-manual/sql-functions/string-functions/extract-url-parameter
No equivalent mysql function i could find.

Continues to address #31508

Changes proposed in this pull request:

  • added test
  • Marked // DORIS ADDED BEGIN|END
  • added parser g4 stuff, within extractUrlParameterFunction
  • added visitor logic as needed
  • updated release notes
  • ran spotless
  • ran full test maven command
  • also ran
cd test/it/parser/
mvn -DskipTests=true generate-sources compile install -q
cd parser/sql/dialect/doris/
mvn test -Dit.test=InternalDorisParserIT

Question

  • do we collapse two consecutive changes? So
// DORIS ADDED END
// DORIS ADDED BEGIN

would be

and have a final terminating
// DORIS ADDED END
at the end


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 (release notes)
  • [✅] 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.1.6 – SELECT EXTRACT_URL_PARAMETER('http://foo.com/?bar=baz','bar')
+---------------------------------------------------------+
| extract_url_parameter('http://foo.com/?bar=baz', 'bar') |
+---------------------------------------------------------+
| baz                                                     |
+---------------------------------------------------------+
1 row in set (0.31 sec)

- added test
- added parser g4 in  `extractUrlParameterFunction`
- added visitor logic
- ran spotless
- ran (though will check again once committed and do a full test)
```
cd test/it/parser/
mvn -DskipTests=true generate-sources compile install -q
cd parser/sql/dialect/doris/
mvn test -Dit.test=InternalDorisParserIT
```

 Question
- do we collapse two consecutive changes?
So
```
// DORIS ADDED END
// DORIS ADDED BEGIN
```

would be
```
```
@danigiri
Copy link
Contributor Author

danigiri commented Nov 6, 2024

Will check the test failures. Looks like the combo:

cd test/it/parser/
mvn -DskipTests=true generate-sources compile install -q
cd parser/sql/dialect/doris/
mvn test -Dit.test=InternalDorisParserIT

doesn't fully test as expected. Currently fully testing on my test server.

@danigiri
Copy link
Contributor Author

danigiri commented Nov 7, 2024

found the correct test indexes, will commit tomorrow

@danigiri
Copy link
Contributor Author

Could complete full test cycle on test machine, next is to incorporate latest upstream changes and should be OK for final review

@danigiri
Copy link
Contributor Author

Ready for review

@strongduanmu
Copy link
Member

Hi @danigiri, can you solve code conflict?

@danigiri
Copy link
Contributor Author

Sure, it is probably the release notes have drifted

@danigiri
Copy link
Contributor Author

@strongduanmu can you check? release notes should be OK now

@danigiri
Copy link
Contributor Author

XML test files have drifted since the PR was created, will resolve conflicts tomorrow morning

@danigiri
Copy link
Contributor Author

As expected, it was test file drift, merged and running checks ATM

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