columnar: Refactor for later pipeline evolution#10907
Open
JaySon-Huang wants to merge 7 commits into
Open
Conversation
…irectory Move Columnar-specific types out of Flash/Coprocessor and StorageDisaggregatedColumnar into a dedicated Storages/Columnar/ directory for better code organization. Changes: - Move ColumnarScanContext from Flash/Coprocessor/ to Storages/Columnar/ - Extract RNColumnarReader* types (Plan/Work/SharedContext/ReadTask) into Storages/Columnar/ColumnarReader.h/.cpp - Extract RNColumnarInputStream and RNColumnarSourceOp into Storages/Columnar/ColumnarStreams.h/.cpp - Simplify StorageDisaggregatedColumnar.h/.cpp to only contain Storage::readThroughColumnar() and related integration glue code - Update all include paths across Flash/Coprocessor and Flash/Statistics - Add Storages/Columnar/ to CMake build glob - Keep ENABLE_NEXT_GEN_COLUMNAR guards on all Columnar code paths
…mbrella header - Rename all Columnar types to drop the 'RN' prefix: RNColumnarReaderMaterializeState -> ColumnarReaderMaterializeState RNColumnarReaderSharedContext -> ColumnarReaderSharedContext RNColumnarReaderPlan -> ColumnarReaderPlan RNColumnarReaderWork -> ColumnarReaderWork RNColumnarReadTask -> ColumnarReadTask RNColumnarInputStream -> ColumnarInputStream RNColumnarSourceOp -> ColumnarSourceOp getRNColumnarSourceNum -> getColumnarSourceNum - Delete StorageDisaggregatedColumnar.h, now redundant since StorageDisaggregatedColumnar.cpp already directly includes the Columnar/ headers
…SourceOp Separate the pull-based and push-based Columnar execution models into independent files for better maintainability and future pipeline optimization: - ColumnarInputStream.h/.cpp: pull-based IProfilingBlockInputStream model (legacy) - ColumnarSourceOp.h/.cpp: push-based SourceOp pipeline model (future) This allows pipeline-specific optimizations (async IO, adaptive concurrency, backpressure) to be scoped to ColumnarSourceOp without touching the legacy InputStream path.
- Rename ColumnarReadTask -> ColumnarReadTaskPool to clarify it is a pool/broker managing multiple ColumnarReaderWork items, not a single task - Shorten static factory methods: buildColumnarReadTaskPool -> build / buildWithBackoff (class name already provides context) - Inline getColumnarSourceNum at its single call site - Inline 8 trivial getter methods into ColumnarReader.h - Add class-level comments for ColumnarReadTaskPool, ColumnarReaderWork, and ColumnarReaderSharedContext - Add member comments for reader_count, source_num, pending_reader_works - Rename local variable read_columnar_tasks -> columnar_task_pools to match the new type name
Move the four behavioral descriptions (backoff/retry, bucket-level parallelism, work-stealing, prefetching) from the ColumnarReadTaskPool class comment to the specific functions that implement each behavior: - buildWithBackoff: backoff/retry on transient errors - build: bucket-level parallelism - tryAcquireReaderWork: work-stealing from the front of the queue - prefetchPendingWork: async prefetch of next work item
Signed-off-by: JaySon-Huang <tshent@qq.com>
After rebasing the refactoring onto upstream/master, apply the makeLockForDisaggResolve change (from pingcap#10900) to createColumnarReader() which was moved from StorageDisaggregatedColumnar.cpp to ColumnarReader.cpp during the refactoring.
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/XXL
📝 WalkthroughWalkthroughAdds next-gen columnar reader infrastructure ( ChangesNext-gen columnar reader pipeline
Sequence Diagram(s)sequenceDiagram
participant StorageDisaggregated
participant ColumnarReadTaskPool
participant ColumnarReaderWork
participant DisaggregatedProxy
rect rgba(70, 130, 180, 0.5)
Note over StorageDisaggregated,DisaggregatedProxy: Planning phase
StorageDisaggregated->>ColumnarReadTaskPool: buildWithBackoff(context, table_ranges, ...)
ColumnarReadTaskPool->>ColumnarReadTaskPool: build() — region split + bucket parallelism
ColumnarReadTaskPool->>ColumnarReaderWork: enqueue per-region/bucket work items
ColumnarReadTaskPool->>ColumnarReadTaskPool: prefetchPendingWork()
ColumnarReadTaskPool->>DisaggregatedProxy: createColumnarReader FFI (serialized region/filter/table)
DisaggregatedProxy-->>ColumnarReaderWork: ColumnarReaderPtr (Ready) or error (Failed)
end
rect rgba(60, 179, 113, 0.5)
Note over StorageDisaggregated,DisaggregatedProxy: Execution phase
StorageDisaggregated->>ColumnarReadTaskPool: getInputStreams() / ColumnarSourceOp::create()
loop each pipeline tick
ColumnarReadTaskPool->>ColumnarReaderWork: tryAcquireReaderWork()
ColumnarReaderWork-->>ColumnarReadTaskPool: ColumnarReaderPtr
ColumnarReadTaskPool->>DisaggregatedProxy: read row batch via proxy
DisaggregatedProxy-->>ColumnarReadTaskPool: column buffers
ColumnarReadTaskPool-->>StorageDisaggregated: Block (deserialized columns)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
dbms/src/Storages/StorageDisaggregatedColumnar.cpp (1)
70-1642: 🧹 Nitpick | 🔵 TrivialConsider removing the old RN implementation code in a follow-up refactoring.*
The old
RN*classes and helper functions (lines 70-1642) are dead code. External verification confirms no usages ofRNColumnarReadTask,RNColumnarSourceOp,RNColumnarInputStream, orRNColumnarReaderSharedContextexist outside this file. These ~1,500 lines were superseded by the migration toColumnarReadTaskPoolandColumnarSourceOpAPIs. Removing this dead code would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp` around lines 70 - 1642, Remove the dead code blocks containing the old RN* implementation that has been superseded by ColumnarReadTaskPool and ColumnarSourceOp APIs. Delete the entire RNColumnarReaderSharedContext struct definition (including the nested StartTsClearRegistry class and all member functions), all helper functions that build and manage RN reader plans (buildColumnarReaderSharedContext, buildRegionReaderPlansFromPhysicalTableRanges, buildReaderPlansFromRegionReaderPlans, createColumnarReader, and related functions), and the complete implementations of RNColumnarReaderWork destructor, RNColumnarReadTask class with all its methods, RNColumnarInputStream class with all its methods, and RNColumnarSourceOp class with all its methods. Before deletion, verify no other code in the repository references these removed classes and functions by searching for RNColumnarReadTask, RNColumnarInputStream, RNColumnarSourceOp, RNColumnarReaderSharedContext, and related identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dbms/src/Flash/Statistics/TableScanImpl.cpp`:
- Line 20: In the file dbms/src/Storages/StorageDisaggregatedColumnar.cpp at
line 27, replace the stale include path from the old header location
<Flash/Coprocessor/ColumnarScanContext.h> with the new correct path
<Storages/Columnar/ColumnarScanContext.h>. This aligns with the updated include
path being used in other files like TableScanImpl.cpp and resolves the
compilation failure due to the deleted header.
In `@dbms/src/Storages/Columnar/ColumnarReader.cpp`:
- Around line 600-613: The ParseFromString call on lock_info is not checking for
parsing failures when deserializing the error message data. In the LockedError
handling block within the ColumnarReader code, verify that
lock_info.ParseFromString(error_msg) returns successfully before proceeding with
lock resolution. If parsing fails, log an appropriate error and throw an
exception or handle the malformed data case to prevent using an invalid
lock_info object in the subsequent resolveLocks call.
- Around line 540-599: The code calls region_error.ParseFromString(error_msg)
but does not check its return value before accessing fields like
has_epoch_not_match() and current_regions(), which causes undefined behavior if
the protobuf data is malformed. Capture the boolean return value of the
ParseFromString call in the RegionError parsing section and add a check to
verify the parse was successful before proceeding to access any fields on the
region_error object. If parsing fails, log an appropriate error and throw a
RegionException instead of continuing with undefined state.
In `@dbms/src/Storages/Columnar/ColumnarReader.h`:
- Around line 136-143: The catch block in the
clear_shared_snap_access_by_start_ts function call is using a basic LOG_WARNING
call instead of the recommended tryLogCurrentException function to handle
exceptions. Replace the LOG_WARNING statement with a tryLogCurrentException call
that includes a descriptive context message about the failure to clear the
shared snapshot access cache. This will ensure proper exception details are
captured for debugging according to the coding guidelines.
In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp`:
- Around line 632-642: The method ColumnarReadTaskPool::buildWithBackoff() is
being called but the required header file is missing from the includes section.
Add the include directive for Storages/Columnar/ColumnarReader.h to the includes
section of StorageDisaggregatedColumnar.cpp (around lines 15-61) to resolve the
compilation error for the ColumnarReadTaskPool class.
---
Nitpick comments:
In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp`:
- Around line 70-1642: Remove the dead code blocks containing the old RN*
implementation that has been superseded by ColumnarReadTaskPool and
ColumnarSourceOp APIs. Delete the entire RNColumnarReaderSharedContext struct
definition (including the nested StartTsClearRegistry class and all member
functions), all helper functions that build and manage RN reader plans
(buildColumnarReaderSharedContext,
buildRegionReaderPlansFromPhysicalTableRanges,
buildReaderPlansFromRegionReaderPlans, createColumnarReader, and related
functions), and the complete implementations of RNColumnarReaderWork destructor,
RNColumnarReadTask class with all its methods, RNColumnarInputStream class with
all its methods, and RNColumnarSourceOp class with all its methods. Before
deletion, verify no other code in the repository references these removed
classes and functions by searching for RNColumnarReadTask,
RNColumnarInputStream, RNColumnarSourceOp, RNColumnarReaderSharedContext, and
related identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e16d20b2-dd50-4133-b713-3be6abb7e89d
📒 Files selected for processing (17)
dbms/CMakeLists.txtdbms/src/Flash/Coprocessor/DAGContext.hdbms/src/Flash/Coprocessor/ExecutionSummary.cppdbms/src/Flash/Coprocessor/ExecutionSummary.hdbms/src/Flash/Statistics/ExecutionSummaryHelper.cppdbms/src/Flash/Statistics/ExecutorStatisticsCollector.cppdbms/src/Flash/Statistics/TableScanImpl.cppdbms/src/Storages/Columnar/ColumnarInputStream.cppdbms/src/Storages/Columnar/ColumnarInputStream.hdbms/src/Storages/Columnar/ColumnarReader.cppdbms/src/Storages/Columnar/ColumnarReader.hdbms/src/Storages/Columnar/ColumnarScanContext.hdbms/src/Storages/Columnar/ColumnarScanContext_fwd.hdbms/src/Storages/Columnar/ColumnarSourceOp.cppdbms/src/Storages/Columnar/ColumnarSourceOp.hdbms/src/Storages/StorageDisaggregatedColumnar.cppdbms/src/Storages/StorageDisaggregatedColumnar.h
💤 Files with no reviewable changes (1)
- dbms/src/Storages/StorageDisaggregatedColumnar.h
| #include <Flash/Statistics/ConnectionProfileInfo.h> | ||
| #include <Flash/Statistics/TableScanImpl.h> | ||
| #include <Interpreters/Join.h> | ||
| #include <Storages/Columnar/ColumnarScanContext.h> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check whether old/new ColumnarScanContext headers exist =="
fd -i 'ColumnarScanContext(_fwd)?\.h$' dbms/src
echo
echo "== Find remaining includes of old path =="
rg -nP '`#include`\s*<Flash/Coprocessor/ColumnarScanContext(_fwd)?\.h>' dbms/src
echo
echo "== Find includes of new path =="
rg -nP '`#include`\s*<Storages/Columnar/ColumnarScanContext(_fwd)?\.h>' dbms/srcRepository: pingcap/tiflash
Length of output: 1230
Fix stale include in dbms/src/Storages/StorageDisaggregatedColumnar.cpp.
The old header <Flash/Coprocessor/ColumnarScanContext.h> no longer exists. Update line 27 of StorageDisaggregatedColumnar.cpp to use <Storages/Columnar/ColumnarScanContext.h> to avoid compilation failure. The file under review correctly uses the new path; one downstream file still references the deleted header and must be corrected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dbms/src/Flash/Statistics/TableScanImpl.cpp` at line 20, In the file
dbms/src/Storages/StorageDisaggregatedColumnar.cpp at line 27, replace the stale
include path from the old header location
<Flash/Coprocessor/ColumnarScanContext.h> with the new correct path
<Storages/Columnar/ColumnarScanContext.h>. This aligns with the updated include
path being used in other files like TableScanImpl.cpp and resolves the
compilation failure due to the deleted header.
Comment on lines
+540
to
+599
| if (columnar_reader.error_type == ColumnarReaderErrorType::RegionError) | ||
| { | ||
| auto error_msg = String(columnar_reader.error.buff.data, columnar_reader.error.buff.len); | ||
| errorpb::Error region_error; | ||
| region_error.ParseFromString(error_msg); | ||
| auto region_ver_id | ||
| = pingcap::kv::RegionVerID(reader_plan.region_id, reader_plan.region_conf_ver, reader_plan.region_ver); | ||
| // Refresh region cache and throw an exception for retrying. | ||
| if (region_error.has_epoch_not_match()) | ||
| { | ||
| RegionException::UnavailableRegions unavailable_regions; | ||
| String region_id_ver; // region_id:region_ver:conf_ver | ||
| for (const auto & region : region_error.epoch_not_match().current_regions()) | ||
| { | ||
| unavailable_regions.insert(region.id()); | ||
| region_id_ver = std::to_string(region.id()) + ":" + std::to_string(reader_plan.region_ver) + ":" | ||
| + std::to_string(region.region_epoch().conf_ver()); | ||
| } | ||
| auto guard = std::lock_guard(*shared_context.output_lock); | ||
| cluster->region_cache->dropRegion(region_ver_id); | ||
| LOG_WARNING( | ||
| log, | ||
| "create columnar reader failed region_id={}, epoch not match {}", | ||
| std::to_string(reader_plan.region_id), | ||
| region_ver_id.toString()); | ||
| throw RegionException( | ||
| std::move(unavailable_regions), | ||
| RegionException::RegionReadStatus::EPOCH_NOT_MATCH, | ||
| region_id_ver.c_str()); | ||
| } | ||
| else | ||
| { | ||
| RegionException::UnavailableRegions unavailable_regions; | ||
| auto err_region_id = 0; | ||
| if (region_error.has_region_not_found()) | ||
| { | ||
| err_region_id = region_error.region_not_found().region_id(); | ||
| unavailable_regions.insert(err_region_id); | ||
| LOG_WARNING( | ||
| log, | ||
| "create columnar reader failed region_id={}, region not found {}", | ||
| std::to_string(reader_plan.region_id), | ||
| std::to_string(err_region_id)); | ||
| } | ||
| else | ||
| { | ||
| LOG_WARNING( | ||
| log, | ||
| "create columnar reader failed region_id={}, {}", | ||
| std::to_string(reader_plan.region_id), | ||
| region_error.ShortDebugString()); | ||
| } | ||
| auto guard = std::lock_guard(*shared_context.output_lock); | ||
| cluster->region_cache->dropRegion(region_ver_id); | ||
| throw RegionException( | ||
| std::move(unavailable_regions), | ||
| RegionException::RegionReadStatus::NOT_FOUND, | ||
| std::to_string(reader_plan.region_id).c_str()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Check ParseFromString return value to avoid undefined behavior on malformed data.
If the FFI layer returns malformed protobuf data, ParseFromString returns false and leaves the message object in an undefined state. Accessing fields like has_epoch_not_match() or current_regions() would then produce undefined behavior.
Proposed fix
if (columnar_reader.error_type == ColumnarReaderErrorType::RegionError)
{
auto error_msg = String(columnar_reader.error.buff.data, columnar_reader.error.buff.len);
errorpb::Error region_error;
- region_error.ParseFromString(error_msg);
+ if (!region_error.ParseFromString(error_msg))
+ {
+ LOG_WARNING(log, "failed to parse region error protobuf, region_id={}", reader_plan.region_id);
+ throw Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "failed to parse region error for region {}", reader_plan.region_id);
+ }
auto region_ver_id🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dbms/src/Storages/Columnar/ColumnarReader.cpp` around lines 540 - 599, The
code calls region_error.ParseFromString(error_msg) but does not check its return
value before accessing fields like has_epoch_not_match() and current_regions(),
which causes undefined behavior if the protobuf data is malformed. Capture the
boolean return value of the ParseFromString call in the RegionError parsing
section and add a check to verify the parse was successful before proceeding to
access any fields on the region_error object. If parsing fails, log an
appropriate error and throw a RegionException instead of continuing with
undefined state.
Comment on lines
+600
to
+613
| else if (columnar_reader.error_type == ColumnarReaderErrorType::LockedError) | ||
| { | ||
| auto error_msg = String(columnar_reader.error.buff.data, columnar_reader.error.buff.len); | ||
| kvrpcpb::LockInfo lock_info; | ||
| lock_info.ParseFromString(error_msg); | ||
| // Try to resolve locks. | ||
| pingcap::kv::Backoffer bo(pingcap::kv::copNextMaxBackoff); | ||
| std::vector<uint64_t> pushed; | ||
| std::vector<pingcap::kv::LockPtr> locks{makeLockForDisaggResolve(lock_info)}; | ||
| auto guard = std::lock_guard(*shared_context.output_lock); | ||
| auto before_expired = cluster->lock_resolver->resolveLocks(bo, shared_context.start_ts, locks, pushed); | ||
| LOG_WARNING(log, "Finished resolve locks, before_expired={}", before_expired); | ||
| throw Exception("lock error", ErrorCodes::COLUMNAR_SNAPSHOT_ERROR); | ||
| } |
There was a problem hiding this comment.
Check ParseFromString return value for lock info.
Same issue as the region error case - parsing could fail on malformed data.
Proposed fix
else if (columnar_reader.error_type == ColumnarReaderErrorType::LockedError)
{
auto error_msg = String(columnar_reader.error.buff.data, columnar_reader.error.buff.len);
kvrpcpb::LockInfo lock_info;
- lock_info.ParseFromString(error_msg);
+ if (!lock_info.ParseFromString(error_msg))
+ {
+ LOG_WARNING(log, "failed to parse lock info protobuf, region_id={}", reader_plan.region_id);
+ throw Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "failed to parse lock info for region {}", reader_plan.region_id);
+ }
// Try to resolve locks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dbms/src/Storages/Columnar/ColumnarReader.cpp` around lines 600 - 613, The
ParseFromString call on lock_info is not checking for parsing failures when
deserializing the error message data. In the LockedError handling block within
the ColumnarReader code, verify that lock_info.ParseFromString(error_msg)
returns successfully before proceeding with lock resolution. If parsing fails,
log an appropriate error and throw an exception or handle the malformed data
case to prevent using an invalid lock_info object in the subsequent resolveLocks
call.
Comment on lines
+136
to
+143
| try | ||
| { | ||
| clear_shared_snap_access_by_start_ts(start_ts, proxy_ptr); | ||
| } | ||
| catch (...) | ||
| { | ||
| LOG_WARNING(log, "clear shared snapaccess cache failed, start_ts={}", start_ts); | ||
| } |
There was a problem hiding this comment.
Use tryLogCurrentException in catch block per coding guidelines.
The broad catch (...) block should use tryLogCurrentException to capture exception details for debugging, rather than a simple log message.
Proposed fix
try
{
clear_shared_snap_access_by_start_ts(start_ts, proxy_ptr);
}
catch (...)
{
- LOG_WARNING(log, "clear shared snapaccess cache failed, start_ts={}", start_ts);
+ tryLogCurrentException(log, fmt::format("clear shared snapaccess cache failed, start_ts={}", start_ts));
}Based on coding guidelines: "Use tryLogCurrentException(log, "context") in broad catch (...) paths to avoid duplicated exception-formatting code"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dbms/src/Storages/Columnar/ColumnarReader.h` around lines 136 - 143, The
catch block in the clear_shared_snap_access_by_start_ts function call is using a
basic LOG_WARNING call instead of the recommended tryLogCurrentException
function to handle exceptions. Replace the LOG_WARNING statement with a
tryLogCurrentException call that includes a descriptive context message about
the failure to clear the shared snapshot access cache. This will ensure proper
exception details are captured for debugging according to the coding guidelines.
Source: Coding guidelines
Comment on lines
+632
to
+642
| auto columnar_task_pools = ColumnarReadTaskPool::buildWithBackoff( | ||
| log, | ||
| context, | ||
| start_ts, | ||
| table_scan, | ||
| filter_conditions, | ||
| remote_table_ranges, | ||
| num_streams); | ||
| for (auto & task : read_columnar_tasks) | ||
| for (auto & task_pool : columnar_task_pools) | ||
| { | ||
| auto streams = task->getInputStreams(); | ||
| auto streams = task_pool->getInputStreams(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the new columnar headers are included
# Check for ColumnarReader.h and ColumnarSourceOp.h includes
rg -n '`#include`.*Columnar(Reader|SourceOp|InputStream)\.h' dbms/src/Storages/StorageDisaggregatedColumnar.cpp
# Also check what headers are currently included
echo "=== All includes in the file ==="
rg -n '^`#include`' dbms/src/Storages/StorageDisaggregatedColumnar.cpp | head -20Repository: pingcap/tiflash
Length of output: 1002
🏁 Script executed:
#!/bin/bash
# Get ALL includes in the file
echo "=== COMPLETE INCLUDE LIST ==="
rg -n '^`#include`' dbms/src/Storages/StorageDisaggregatedColumnar.cpp
# Find where ColumnarReadTaskPool is defined
echo ""
echo "=== LOCATING ColumnarReadTaskPool DEFINITION ==="
rg -r '$MATCH' 'class ColumnarReadTaskPool' --type cpp --type h | head -5
# Search for ColumnarReadTaskPool in header files
echo ""
echo "=== FILES DECLARING ColumnarReadTaskPool ==="
fd --type f '\.(h|hpp)$' | xargs rg -l 'class ColumnarReadTaskPool|ColumnarReadTaskPool\s*::'Repository: pingcap/tiflash
Length of output: 2216
Add missing header for ColumnarReadTaskPool.
The code uses ColumnarReadTaskPool::buildWithBackoff() at line 632, but the required header #include <Storages/Columnar/ColumnarReader.h> is not present in the includes section (lines 15-61). Add this include to enable compilation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp` around lines 632 - 642,
The method ColumnarReadTaskPool::buildWithBackoff() is being called but the
required header file is missing from the includes section. Add the include
directive for Storages/Columnar/ColumnarReader.h to the includes section of
StorageDisaggregatedColumnar.cpp (around lines 15-61) to resolve the compilation
error for the ColumnarReadTaskPool class.
Contributor
|
@JaySon-Huang: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
1 participant
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.
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Refactor