Skip to content

[MINOR][CH] Correct misleading Spark 3.2 comment in ReadBufferBuilder#12583

Open
LuciferYang wants to merge 2 commits into
apache:mainfrom
LuciferYang:gluten-minor-ch-fix-readbufferbuilder-comment
Open

[MINOR][CH] Correct misleading Spark 3.2 comment in ReadBufferBuilder#12583
LuciferYang wants to merge 2 commits into
apache:mainfrom
LuciferYang:gluten-minor-ch-fix-readbufferbuilder-comment

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Final follow-up in the Spark 3.2 cleanup series (#12525, #12541, #12550; open #12543 / #12548 / #12574).

The fallback path in ReadBufferBuilder.cpp at

if (!file_size.has_value())
{
    // only for spark3.2 file partition not contained file size
    // so first compute file size first
    auto tmp_read_buffer = std::make_unique<DB::ReadBufferFromHDFS>(...);
    file_size = tmp_read_buffer->getFileSize();
}

is still triggered on Spark 3.3+ whenever the Substrait LocalFilesNode was built without a fileSizes list — e.g. by IcebergLocalFilesBuilder — so the comment "only for spark3.2" is factually wrong on the currently supported version matrix.

This PR rewords the comment to describe the actual condition. The fallback code itself is unchanged.

How was this patch tested?

  • Comment-only change — no code path touched.
  • Verified via gluten-substrait/.../LocalFilesNode.java:194-203 that the fileProperties.setFileSize(...) block only fires when the caller passes a non-empty fileSizes list; producers such as IcebergLocalFilesBuilder do not pass one, so the fallback is unavoidable on Spark 3.3+.
  • Repo-wide grep -rn 'spark3.2\|Spark 3.2' cpp-ch: 0 remaining occurrences after this change.
  • clang-format was not run: the local toolchain is clang-format 12.0.1 and the project requires clang-format-15; running the mismatched version would produce a diff CI rejects (see AGENTS.md).

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

Generated-by: Claude claude-opus-4-7

The fallback path at `if (!file_size.has_value()) { ... }` was commented
as "only for spark3.2 file partition not contained file size", but the
fallback is still triggered on Spark 3.3+ whenever the Substrait
`LocalFilesNode` was built without a `fileSizes` list — e.g. by
`IcebergLocalFilesBuilder`. Reword the comment to describe the actual
condition. No code change.
Copilot AI review requested due to automatic review settings July 21, 2026 03:59
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates a misleading Spark 3.2-specific comment in the ClickHouse backend’s ReadBufferBuilder.cpp to better reflect when the file-size fallback path is taken (i.e., when Substrait LocalFiles inputs omit file size metadata), without changing runtime behavior.

Changes:

  • Reworded the HDFS file-size fallback comment to be version-agnostic and describe the real triggering condition.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cpp-ch/local-engine/Storages/SubstraitSource/ReadBufferBuilder.cpp Outdated
Address Copilot review feedback on apache#12583: distinguish the producer-side
identifier (`fileSizes` list in `LocalFilesNode`) from the per-file
protobuf property (`fileSize`) so the comment matches the code.
Copilot AI review requested due to automatic review settings July 21, 2026 04:41
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

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.

2 participants