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.

Sign up for GitHub

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

Jump to bottom

Scalar function for url encoding and decoding #8378

Merged
merged 4 commits into from
Mar 22, 2022
Merged

Scalar function for url encoding and decoding #8378

merged 4 commits into from
Mar 22, 2022

Conversation

jasperjiaguo
Copy link
Contributor

Description

Add scalar functions to perform url encoding and decoding in queries.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
No

@jasperjiaguo jasperjiaguo changed the title Scalar function for url encoding and decoding [WIP] Scalar function for url encoding and decoding Mar 21, 2022
@jasperjiaguo jasperjiaguo marked this pull request as draft March 21, 2022 17:30
@codecov-commenter
Copy link

Codecov Report

Merging #8378 (00c545e) into master (24d4fd2) will decrease coverage by 9.72%.
The diff coverage is 69.09%.

❗ Current head 00c545e differs from pull request most recent head 6dcee76. Consider uploading reports for the commit 6dcee76 to get more accurate results

@@             Coverage Diff              @@
##             master    #8378      +/-   ##
============================================
- Coverage     70.79%   61.07%   -9.73%     
+ Complexity     4264     4192      -72     
============================================
  Files          1640     1640              
  Lines         85931    86228     +297     
  Branches      12922    13035     +113     
============================================
- Hits          60837    52664    -8173     
- Misses        20899    29679    +8780     
+ Partials       4195     3885     -310
Flag Coverage Δ
integration1 ?
integration2 27.26% <24.06%> (-0.27%) ⬇️
unittests1 67.03% <62.45%> (+0.08%) ⬆️
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...roker/requesthandler/GrpcBrokerRequestHandler.java 75.67% <ø> (ø)
...inot/controller/helix/ControllerRequestClient.java 0.00% <0.00%> (ø)
...manager/realtime/LLRealtimeSegmentDataManager.java 69.70% <ø> (-1.99%) ⬇️
...ion/tasks/BaseSingleSegmentConversionExecutor.java 0.00% <0.00%> (-74.61%) ⬇️
...ot/plugin/minion/tasks/SegmentConversionUtils.java 38.33% <ø> (-38.34%) ⬇️
...he/pinot/segment/local/utils/SegmentPushUtils.java 13.48% <0.00%> (ø)
...e/pinot/core/query/reduce/RowBasedBlockValSet.java 16.12% <16.12%> (ø)
...pache/pinot/common/utils/request/RequestUtils.java 86.39% <33.33%> (-1.11%) ⬇️
...org/apache/pinot/common/utils/http/HttpClient.java 44.65% <44.65%> (ø)
...e/pinot/common/utils/FileUploadDownloadClient.java 54.80% <61.42%> (-8.21%) ⬇️
... and 329 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24d4fd2...6dcee76. Read the comment docs.

siddharthteotia
siddharthteotia reviewed Mar 21, 2022
View reviewed changes
...roker/src/test/java/org/apache/pinot/broker/requesthandler/LiteralOnlyBrokerRequestTest.java
@@ -94,6 +94,15 @@ public void testLiteralOnlyTransformBrokerRequestFromSQL() {
.compileToPinotQuery("SELECT ago('PT1H'), fromDateTime('2020-01-01 UTC', 'yyyy-MM-dd z') FROM myTable")));
Assert.assertFalse(BaseBrokerRequestHandler
.isLiteralOnlyQuery(CalciteSqlParser.compileToPinotQuery("SELECT count(*) from foo where bar > ago('PT1H')")));
Assert.assertTrue(BaseBrokerRequestHandler.isLiteralOnlyQuery(CalciteSqlParser
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not use full URLs as inputs to encodeUrl / decodeUrl functions in tests ? From user perspective, they will use the full URL right ?

SELECT encodeUrl(<myURL>) FROM FOO .....

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasperjiaguo - please add test cases with real URLs as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I merged the PR too soon. @jasperjiaguo Can you please address Sidd's comments in a separate PR?

@siddharthteotia siddharthteotia marked this pull request as ready for review March 22, 2022 00:26
@siddharthteotia siddharthteotia changed the title [WIP] Scalar function for url encoding and decoding Scalar function for url encoding and decoding Mar 22, 2022
Jackie-Jiang
Jackie-Jiang approved these changes Mar 22, 2022
View reviewed changes
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/StringFunctions.java Show resolved Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/StringFunctions.java Show resolved Hide resolved
@Jackie-Jiang Jackie-Jiang merged commit 7cb2c9c into apache:master Mar 22, 2022
@jasperjiaguo jasperjiaguo mentioned this pull request Mar 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siddharthteotia siddharthteotia siddharthteotia left review comments

@Jackie-Jiang Jackie-Jiang Jackie-Jiang approved these changes

@xiangfu0 xiangfu0 Awaiting requested review from xiangfu0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jasperjiaguo @codecov-commenter @siddharthteotia @Jackie-Jiang