Skip to content

Scalar function for url encoding and decoding#8378

Merged
Jackie-Jiang merged 4 commits intoapache:masterfrom
jasperjiaguo:url_encoding
Mar 22, 2022
Merged

Scalar function for url encoding and decoding#8378
Jackie-Jiang merged 4 commits intoapache:masterfrom
jasperjiaguo:url_encoding

Conversation

@jasperjiaguo
Copy link
Contributor

@jasperjiaguo jasperjiaguo commented Mar 21, 2022

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-commenter commented Mar 21, 2022

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
.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

@siddharthteotia siddharthteotia Mar 21, 2022

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

@siddharthteotia siddharthteotia Mar 22, 2022

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

@Jackie-Jiang Jackie-Jiang Mar 22, 2022

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

@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