Skip to content

misc(native): Clean up SystemConnector: fix fragile statics, macro hygiene, const-correctness, and switch safety#27251

Merged
amitkdutta merged 1 commit intoprestodb:masterfrom
amitkdutta:export-D95012494
Mar 3, 2026
Merged

misc(native): Clean up SystemConnector: fix fragile statics, macro hygiene, const-correctness, and switch safety#27251
amitkdutta merged 1 commit intoprestodb:masterfrom
amitkdutta:export-D95012494

Conversation

@amitkdutta
Copy link
Copy Markdown
Contributor

@amitkdutta amitkdutta commented Mar 3, 2026

Summary:
Several code quality issues in SystemConnector accumulated over time:

B1: Fix std::move from static local vectors in taskSchema()
kTaskColumnNames and kTaskColumnTypes were mutable static vectors that got std::moved into the ROW() constructor. After the first call, these vectors would be in a moved-from (empty) state. The code worked by accident because kTaskSchema is also static and initialized once, but it was fragile and confusing. Replaced with a single static const RowTypePtr constructed directly.

B2: Fix macro copy-by-value and add #undef
SET_TASK_COLUMN and SET_TASK_FMT_COLUMN macros copied task (shared_ptr) and taskInfo (TaskInfo struct) by value on every loop iteration. Changed to const auto& references with [[maybe_unused]] since each macro expansion may use only one of the two variables. Added #undef after use.

B4: Add const to SystemSplit accessors
schemaName() and tableName() returned const std::string& but were not const-qualified, preventing use on const SystemSplit&.

B5: Add default case to getTaskResults() switch
The switch over TaskColumnEnum had no default clause. Adding a new column enum without updating the switch would silently skip population. Added default: VELOX_UNREACHABLE().

== NO RELEASE NOTE ==

Summary by Sourcery

Improve SystemConnector code safety and const-correctness for task metadata handling.

Bug Fixes:

  • Replace fragile static mutable task schema vectors with a single static const RowType definition.
  • Ensure getTaskResults() handles future TaskColumnEnum additions by adding a default switch case that fails fast.

Enhancements:

  • Avoid unnecessary copying in task column helper macros by using referenced task/taskInfo variables and cleaning up macro scope with undef directives.
  • Make SystemSplit schemaName() and tableName() accessors const-qualified to support use on const instances.

…giene, const-correctness, and switch safety

Summary:
Several code quality issues in SystemConnector accumulated over time:

**B1: Fix std::move from static local vectors in taskSchema()**
`kTaskColumnNames` and `kTaskColumnTypes` were mutable `static` vectors that got `std::move`d into the `ROW()` constructor. After the first call, these vectors would be in a moved-from (empty) state. The code worked by accident because `kTaskSchema` is also static and initialized once, but it was fragile and confusing. Replaced with a single `static const RowTypePtr` constructed directly.

**B2: Fix macro copy-by-value and add #undef**
`SET_TASK_COLUMN` and `SET_TASK_FMT_COLUMN` macros copied `task` (shared_ptr) and `taskInfo` (TaskInfo struct) by value on every loop iteration. Changed to `const auto&` references with `[[maybe_unused]]` since each macro expansion may use only one of the two variables. Added `#undef` after use.

**B4: Add const to SystemSplit accessors**
`schemaName()` and `tableName()` returned `const std::string&` but were not `const`-qualified, preventing use on `const SystemSplit&`.

**B5: Add default case to getTaskResults() switch**
The switch over `TaskColumnEnum` had no `default` clause. Adding a new column enum without updating the switch would silently skip population. Added `default: VELOX_UNREACHABLE()`.

Differential Revision: D95012494
@amitkdutta amitkdutta requested review from a team as code owners March 3, 2026 06:34
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Mar 3, 2026
@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai bot commented Mar 3, 2026

Reviewer's Guide

Refactors SystemConnector task schema construction and task population macros to be safer and more efficient, improves const-correctness of SystemSplit accessors, and hardens the task results switch with an explicit unreachable default.

Class diagram for updated SystemConnector components

classDiagram
  class SystemTableHandle {
    +RowTypePtr taskSchema() const
  }

  class SystemDataSource {
    +RowVectorPtr getTaskResults()
    +std::optional~RowVectorPtr~ next(uint64_t size, ContinueFuture& future)
  }

  class SystemSplit {
    +SystemSplit(const std::string& schemaName, const std::string& tableName)
    +const std::string& schemaName() const
    +const std::string& tableName() const
    -std::string schemaName_
    -std::string tableName_
  }

  SystemDataSource ..> SystemSplit : uses_splits
  SystemTableHandle ..> SystemDataSource : defines_schema_for
Loading

File-Level Changes

Change Details Files
Make taskSchema() use a single static const RowTypePtr instead of mutable static vectors that are moved-from.
  • Inline the task column names directly into the ROW() call as an initializer list.
  • Inline the task column types directly into the ROW() call using velox::VARCHAR/BIGINT/TIMESTAMP factories.
  • Remove mutable static std::vector instances and std::move usage, leaving a single static const RowTypePtr kTaskSchema.
presto-native-execution/presto_cpp/main/connectors/SystemConnector.cpp
Improve macro hygiene and efficiency for task column population.
  • Change SET_TASK_COLUMN to bind task and taskInfo as [[maybe_unused]] const auto& instead of copying by value.
  • Change SET_TASK_FMT_COLUMN similarly and keep a reusable std::string temp for formatting.
  • Add VELOX_UNREACHABLE() as the default case in the TaskColumnEnum switch inside getTaskResults().
  • Add #undef for SET_TASK_COLUMN and SET_TASK_FMT_COLUMN after their last use to avoid macro leakage.
presto-native-execution/presto_cpp/main/connectors/SystemConnector.cpp
Make SystemSplit accessors const-correct.
  • Mark schemaName() as a const method returning const std::string&.
  • Mark tableName() as a const method returning const std::string&.
presto-native-execution/presto_cpp/main/connectors/SystemSplit.h

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@amitkdutta amitkdutta merged commit caab121 into prestodb:master Mar 3, 2026
82 of 86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants