misc(native): Fix lint issues in presto_cpp types/ directory#27194
Merged
amitkdutta merged 1 commit intoprestodb:masterfrom Feb 24, 2026
Merged
misc(native): Fix lint issues in presto_cpp types/ directory#27194amitkdutta merged 1 commit intoprestodb:masterfrom
amitkdutta merged 1 commit intoprestodb:masterfrom
Conversation
Contributor
Reviewer's GuideThis PR resolves CLANGTIDY and INFERMINICPP lint warnings in the Presto native types module by tightening includes, modernizing string and filesystem usage, fixing function signatures (removing meaningless const and adding noexcept), and avoiding unnecessary copies in query plan conversion code. Class diagram for updated TypeParser and query plan converter usageclassDiagram
class TypeParser {
+parse(text string) velox_TypePtr
}
class SystemConfig {
+instance() SystemConfigPtr
+charNToVarcharImplicitCast() bool
}
class VeloxQueryPlanConverterBase {
+generateOutputVariables(statisticsAggregation StatisticsAggregationPtr, outputVariables vector_Variable) vector_Variable
+toSortFieldsAndOrders(orderingScheme OrderingSchemePtr) void
}
class StatisticsAggregation {
+outputVariables vector_Variable
+groupingVariables vector_Variable
}
class OrderingScheme {
+orderBy vector_VariableSortOrderPair
}
class vector_Variable
class vector_VariableSortOrderPair
TypeParser --> SystemConfig : uses
VeloxQueryPlanConverterBase --> StatisticsAggregation : reads_by_const_reference
VeloxQueryPlanConverterBase --> OrderingScheme : reads_by_const_reference
StatisticsAggregation --> vector_Variable : contains
OrderingScheme --> vector_VariableSortOrderPair : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The newly added
noexceptqualifiers ongetDataPathandapplyClionDirFixlook risky since these functions call APIs likeboost::filesystem::current_path()and perform string allocations/concatenations that can throw; consider droppingnoexceptor tightening the implementations to guarantee they truly cannot throw.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The newly added `noexcept` qualifiers on `getDataPath` and `applyClionDirFix` look risky since these functions call APIs like `boost::filesystem::current_path()` and perform string allocations/concatenations that can throw; consider dropping `noexcept` or tightening the implementations to guarantee they truly cannot throw.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
d2af1c4 to
b28eeda
Compare
amitkdutta
added a commit
to amitkdutta/presto
that referenced
this pull request
Feb 24, 2026
…b#27194) Summary: Fix CLANGTIDY and INFERMINICPP lint warnings across the types/ directory: TestUtils.cpp: - Replace broad boost includes (string.hpp, filesystem.hpp) with specific headers - Add missing #include <string> - Remove unused `using namespace std;` - Remove meaningless `const` qualifier on return-by-value types - Replace redundant `.c_str()` with `.string()` on boost::filesystem::current_path() - Add `noexcept` to function declarations and definitions TestUtils.h: - Add `#pragma once` header guard - Add missing `#include <string>` - Remove `const` from return types and add `noexcept` TypeParser.cpp: - Remove unused includes: `boost/algorithm/string.hpp` and `iostream` - Fix include ordering (self-header first) - Use `starts_with()` instead of `find() == 0` (modernize-use-starts-ends-with) TypeParser.h: - Add missing `#include <string>` and `#include <unordered_map>` PrestoTaskId.h: - Add missing `#include <cstdint>`, `#include <vector>` - Fix include ordering (standard, then third-party, then project) PrestoToVeloxQueryPlan.cpp: - Fix unnecessary copies: use `const auto&` for `statisticsOutputVariables` and `nodeSpecOrdering` Differential Revision: D94156498
amitkdutta
added a commit
to amitkdutta/presto
that referenced
this pull request
Feb 24, 2026
…b#27194) Summary: Fix CLANGTIDY and INFERMINICPP lint warnings across the types/ directory: TestUtils.cpp: - Replace broad boost includes (string.hpp, filesystem.hpp) with specific headers - Add missing #include <string> - Remove unused `using namespace std;` - Remove meaningless `const` qualifier on return-by-value types - Replace redundant `.c_str()` with `.string()` on boost::filesystem::current_path() - Add `noexcept` to function declarations and definitions TestUtils.h: - Add `#pragma once` header guard - Add missing `#include <string>` - Remove `const` from return types and add `noexcept` TypeParser.cpp: - Remove unused includes: `boost/algorithm/string.hpp` and `iostream` - Fix include ordering (self-header first) - Use `starts_with()` instead of `find() == 0` (modernize-use-starts-ends-with) TypeParser.h: - Add missing `#include <string>` and `#include <unordered_map>` PrestoTaskId.h: - Add missing `#include <cstdint>`, `#include <vector>` - Fix include ordering (standard, then third-party, then project) PrestoToVeloxQueryPlan.cpp: - Fix unnecessary copies: use `const auto&` for `statisticsOutputVariables` and `nodeSpecOrdering` Differential Revision: D94156498
b28eeda to
7324b3b
Compare
Summary: Fix CLANGTIDY and INFERMINICPP lint warnings across the types/ directory: TestUtils.cpp: - Replace broad boost includes (string.hpp, filesystem.hpp) with specific headers - Add missing #include <string> - Remove unused `using namespace std;` - Remove meaningless `const` qualifier on return-by-value types - Replace redundant `.c_str()` with `.string()` on boost::filesystem::current_path() - Add `noexcept` to function declarations and definitions TestUtils.h: - Add `#pragma once` header guard - Add missing `#include <string>` - Remove `const` from return types and add `noexcept` TypeParser.cpp: - Remove unused includes: `boost/algorithm/string.hpp` and `iostream` - Fix include ordering (self-header first) - Use `starts_with()` instead of `find() == 0` (modernize-use-starts-ends-with) TypeParser.h: - Add missing `#include <string>` and `#include <unordered_map>` PrestoTaskId.h: - Add missing `#include <cstdint>`, `#include <vector>` - Fix include ordering (standard, then third-party, then project) PrestoToVeloxQueryPlan.cpp: - Fix unnecessary copies: use `const auto&` for `statisticsOutputVariables` and `nodeSpecOrdering` Differential Revision: D94156498
7324b3b to
56545a3
Compare
gggrace14
approved these changes
Feb 24, 2026
This was referenced Mar 31, 2026
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
Fix CLANGTIDY and INFERMINICPP lint warnings across the types/ directory:
TestUtils.cpp:
using namespace std;constqualifier on return-by-value types.c_str()with.string()on boost::filesystem::current_path()noexceptto function declarations and definitionsTestUtils.h:
#pragma onceheader guard#include <string>constfrom return types and addnoexceptTypeParser.cpp:
boost/algorithm/string.hppandiostreamstarts_with()instead offind() == 0(modernize-use-starts-ends-with)TypeParser.h:
#include <string>and#include <unordered_map>PrestoTaskId.h:
#include <cstdint>,#include <vector>PrestoToVeloxQueryPlan.cpp:
const auto&forstatisticsOutputVariablesandnodeSpecOrderingSummary by Sourcery
Resolve static analysis and lint warnings in the presto_cpp types module and associated tests.
Enhancements:
Tests: