fix: Update native protocol code updates and apply format fixes missed in previous PRs. - #43
Conversation
WalkthroughStylistic and formatting improvements were made to several CMakeLists.txt files without affecting build logic. In the Hive protocol, a new optional field, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Table (Hive Protocol)
participant JSON
User->>Table (Hive Protocol): Set or get Table object (with optional catalogName)
Table (Hive Protocol)->>JSON: to_json(Table) serializes catalogName (if present)
JSON->>Table (Hive Protocol): from_json(JSON) deserializes catalogName (if present)
Estimated code review effort2 (~15 minutes) Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| # Set all Velox options below | ||
| # Make sure that if we include folly headers or other dependency headers | ||
| # that include folly headers we turn off the coroutines and turn on int128. | ||
| # Set all Velox options below. | ||
|
|
||
| # Make sure that if we include folly headers or other dependency headers that | ||
| # include folly headers we turn off the coroutines and turn on int128. |
There was a problem hiding this comment.
I think the orignal comments (in prestodb master branch) are not formatted well so the clang-format is trying to fix this issue. I changed it a bit according to this line: https://github.com/y-scope/presto/pull/43/files#diff-f4770996c945761a121b4351ae31adecae46b00a0767c8c4cb04112731984795R45
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
presto-native-execution/CMakeLists.txt(3 hunks)presto-native-execution/presto_cpp/main/CMakeLists.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
presto-native-execution/CMakeLists.txt (2)
Learnt from: anlowee
PR: #7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.
Learnt from: anlowee
PR: y-scope/presto#0
File: :0-0
Timestamp: 2025-06-19T08:41:46.288Z
Learning: In the y-scope/presto repository, prefer static imports for commonly used methods like String.format(). Instead of writing String.format("%s", value), use static import import static java.lang.String.format; and call format("%s", value) directly.
🔇 Additional comments (4)
presto-native-execution/presto_cpp/main/CMakeLists.txt (1)
204-205: LGTM – split long list for readability only.The re-wrapped
set(PROXYGEN_LIBRARIES …)preserves the same contents; no build impact.presto-native-execution/CMakeLists.txt (3)
32-40: LGTM – comment reflow only.The bullet list describing disabled warnings is clearer now; build flags unchanged.
73-76: LGTM – clarified Velox options comment.Purely stylistic; no functional impact.
204-205: LGTM – library list formatting only.Line split improves readability; link order preserved.
| # velox_tpch_connector is an OBJECT target in Velox and so needs to be linked to | ||
| # the executable or use TARGET_OBJECT linkage for the presto_server_lib target. | ||
| # However, we also would need to add its dependencies (tpch_gen etc). | ||
| # TODO change the target in Velox to a library target then we can move this to the | ||
| # | ||
| # TODO change the target in Velox to a library target then we can move this to | ||
| # the presto_server_lib. | ||
| # presto_server_lib. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove stray duplicate comment line.
Line 120 repeats the fragment “presto_server_lib.” that is already covered in the previous sentence and adds no value. Trimming it keeps the comment block tidy.
-# presto_server_lib.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # velox_tpch_connector is an OBJECT target in Velox and so needs to be linked to | |
| # the executable or use TARGET_OBJECT linkage for the presto_server_lib target. | |
| # However, we also would need to add its dependencies (tpch_gen etc). | |
| # TODO change the target in Velox to a library target then we can move this to the | |
| # | |
| # TODO change the target in Velox to a library target then we can move this to | |
| # the presto_server_lib. | |
| # presto_server_lib. | |
| # velox_tpch_connector is an OBJECT target in Velox and so needs to be linked to | |
| # the executable or use TARGET_OBJECT linkage for the presto_server_lib target. | |
| # However, we also would need to add its dependencies (tpch_gen etc). | |
| # | |
| # TODO change the target in Velox to a library target then we can move this to | |
| # the presto_server_lib. |
🤖 Prompt for AI Agents
In presto-native-execution/presto_cpp/main/CMakeLists.txt around lines 114 to
120, remove the duplicate fragment "presto_server_lib." on line 120 from the
comment block to avoid redundancy and keep the comments clean and concise.
anlowee
changed the title
3 participants
Description
Typically we should run
make format-fixunderpresto-native-execution/when we make any Presttimo changes. However, we didn't run it for the last few PRs.When update the protocol, we should run
make presto_protocolunderpresto-native-execution/presto_cpp/presto_protocolinstead of manually run the code generation commands. Since we forgot to do so, there were some hive connector related protocol changes didn't get updated correctly.In this PR, we run
make format-fixandmake presto_protocolto fix the format issues as well as the protocol inconsistency.Checklist
breaking change.
Validation performed
Passed the CI.
Summary by CodeRabbit
New Features
Style