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

[feat][sql] Bump Trino version to 368 and fix Decimal breaking change #20016

Merged
merged 7 commits into from
Apr 10, 2023

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Apr 5, 2023

This is a follow-up to #16683.

And it fixes one of the breaking changes noted at #16494.

The final goal should be catching up to the latest trino version. For compatibility, the Trino community says:

We generally do our best to avoid making breaking changes in SPI, but we still do them from time to time.

There is a revapi test in SPI which provides compatibility guarantees one version back, but we sometimes have to do exclusions there to enable innovation.

TL;DR is it’s recommended to build a plugin version for a specific Trino version

So it seems the breaking changes and incompatibility is how Trino users should be familiar with.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@tisonkun tisonkun self-assigned this Apr 5, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 5, 2023
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
@tisonkun tisonkun marked this pull request as draft April 6, 2023 16:52
@tisonkun
Copy link
Member Author

tisonkun commented Apr 6, 2023

Still some compatibility issue. Convert to draft for now.

@tisonkun tisonkun marked this pull request as ready for review April 6, 2023 19:16
@codecov-commenter
Copy link

Codecov Report

Merging #20016 (ddc4f07) into master (8c50a6c) will increase coverage by 39.67%.
The diff coverage is 51.75%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20016       +/-   ##
=============================================
+ Coverage     33.17%   72.85%   +39.67%     
- Complexity    12158    31628    +19470     
=============================================
  Files          1499     1861      +362     
  Lines        113832   137492    +23660     
  Branches      12368    15145     +2777     
=============================================
+ Hits          37769   100170    +62401     
+ Misses        71143    29346    -41797     
- Partials       4920     7976     +3056     
Flag Coverage Δ
inttests 24.35% <0.00%> (?)
systests 25.04% <0.00%> (?)
unittests 72.12% <51.75%> (+38.94%) ⬆️

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

Impacted Files Coverage Δ
...tensions/filter/AntiAffinityGroupPolicyFilter.java 83.33% <ø> (+83.33%) ⬆️
...e/extensions/filter/BrokerMaxTopicCountFilter.java 87.50% <ø> (+87.50%) ⬆️
...balance/extensions/filter/BrokerVersionFilter.java 80.39% <ø> (+80.39%) ⬆️
...nsions/policies/AntiAffinityGroupPolicyHelper.java 77.27% <0.00%> (+77.27%) ⬆️
...pulsar/broker/admin/impl/PersistentTopicsBase.java 64.18% <18.18%> (+54.83%) ⬆️
...ql/presto/decoder/json/PulsarJsonFieldDecoder.java 63.02% <45.45%> (ø)
...l/presto/decoder/avro/PulsarAvroColumnDecoder.java 60.41% <50.00%> (ø)
...tensions/filter/BrokerIsolationPoliciesFilter.java 88.88% <66.66%> (+88.88%) ⬆️
...pulsar/sql/presto/PulsarSqlSchemaInfoProvider.java 68.00% <85.71%> (ø)
...dbalance/extensions/scheduler/TransferShedder.java 83.07% <86.20%> (+83.07%) ⬆️
... and 3 more

... and 1520 files with indirect coverage changes

@tisonkun
Copy link
Member Author

tisonkun commented Apr 7, 2023

@merlimat @Technoboy- This patch should be ready to review.

The only failed CI is about Spring dep CVE. We actually filter out all pulsar-sql when running OWASP so it should be unrelated.

@tisonkun tisonkun requested a review from MarvinCai April 7, 2023 02:03
@tisonkun
Copy link
Member Author

Merging...

Thanks for your review.

@tisonkun tisonkun merged commit 470b674 into apache:master Apr 10, 2023
@tisonkun tisonkun deleted the bump-trino-368 branch April 10, 2023 07:06
@tisonkun tisonkun mentioned this pull request Apr 10, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants