Skip to content

[MINOR][VL] Fix clang-tidy warnings in WholeStageResultIterator - #13356

Open
malinjawi wants to merge 1 commit into
apache:mainfrom
malinjawi:fix/whole-stage-iterator-tidy
Open

malinjawi wants to merge 1 commit into
apache:mainfrom
malinjawi:fix/whole-stage-iterator-tidy

Conversation

@malinjawi

@malinjawi malinjawi commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

CI's clang-tidy step lints every file a PR touches, so any change to WholeStageResultIterator shows these five warnings (they came up on #13165):

  • spillDir is passed by const reference instead of by value.
  • projectSource is bound by reference instead of copying the shared_ptr.
  • Each partial aggregation memory limit is read once into an optional and checked before it is dereferenced, instead of calling get() twice. The ratio fallback is still read only when the limit is not set.
  • The destructor now catches and logs a failure to cancel or wait for the Velox task. Before, such an exception would call std::terminate.

Nothing else changes.

How was this patch tested?

Existing native tests in CI. clang-format-15 clean.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: IBM Bob

CI's clang-tidy step lints every file a PR touches, so these five
warnings show up on any change to WholeStageResultIterator:

- pass spillDir by const reference;
- bind projectSource by reference instead of copying the shared_ptr;
- read each partial aggregation memory limit into one optional and check
  it before dereferencing, instead of calling get() twice; the ratio
  fallback is still read only when the limit is not set;
- catch and log a failure to cancel or wait for the Velox task in the
  destructor, which would otherwise call std::terminate.
@github-actions github-actions Bot added the VELOX label Oct 18, 2026
@malinjawi

Copy link
Copy Markdown
Contributor Author

cc: @zhouyuan

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant