Copilot commented on code in PR #61042:
URL: https://github.com/apache/doris/pull/61042#discussion_r2883132849
##########
be/src/vec/exec/scan/es_scanner.cpp:
##########
@@ -78,7 +78,7 @@ Status EsScanner::init(RuntimeState* state, const
VExprContextSPtrs& conjuncts)
return Status::OK();
}
-Status EsScanner::open(RuntimeState* state) {
+Status EsScanner::_open_impl(RuntimeState* state) {
VLOG_CRITICAL << NEW_SCANNER_TYPE << "::open";
Review Comment:
This log still uses `::open` even though the method is now `_open_impl()`.
Updating it will avoid confusion when correlating logs with stack traces or
symbols.
```suggestion
VLOG_CRITICAL << NEW_SCANNER_TYPE << "::_open_impl";
```
##########
be/src/vec/exec/scan/olap_scanner.h:
##########
@@ -75,7 +75,7 @@ class OlapScanner : public Scanner {
Status prepare() override;
- Status open(RuntimeState* state) override;
+ Status _open_impl(RuntimeState* state) override;
Review Comment:
`_open_impl()` is declared in the public section, which allows callers to
bypass `Scanner::open()` (and therefore bypass the `SCOPED_RAW_TIMER`
instrumentation) and undermines the intended NVI pattern. Consider moving
`_open_impl()` into `protected:` (similar to `_get_block_impl()`), keeping
`open()` as the only public entry point.
##########
be/src/vec/exec/scan/jdbc_scanner.cpp:
##########
@@ -101,7 +101,7 @@ Status JdbcScanner::init(RuntimeState* state, const
VExprContextSPtrs& conjuncts
return Status::OK();
}
-Status JdbcScanner::open(RuntimeState* state) {
+Status JdbcScanner::_open_impl(RuntimeState* state) {
VLOG_CRITICAL << "JdbcScanner::open";
Review Comment:
The log line still prints `JdbcScanner::open`, but the implementation has
been renamed to `_open_impl()`. Please update the log message so it reflects
the correct function and is easier to grep/debug.
```suggestion
VLOG_CRITICAL << "JdbcScanner::_open_impl";
```
##########
be/src/vec/exec/scan/meta_scanner.cpp:
##########
@@ -64,9 +64,9 @@ MetaScanner::MetaScanner(RuntimeState* state,
pipeline::ScanLocalStateBase* loca
_user_identity(user_identity),
_scan_range(scan_range.scan_range) {}
-Status MetaScanner::open(RuntimeState* state) {
+Status MetaScanner::_open_impl(RuntimeState* state) {
VLOG_CRITICAL << "MetaScanner::open";
Review Comment:
This log message still says `MetaScanner::open` even though the method was
renamed to `_open_impl()`. Updating the string will make logs match the actual
call path (and avoid confusion when debugging).
```suggestion
VLOG_CRITICAL << "MetaScanner::_open_impl";
```
##########
be/src/vec/exec/scan/jdbc_scanner.h:
##########
@@ -48,7 +48,7 @@ class JdbcScanner : public Scanner {
JdbcScanner(RuntimeState* state, doris::pipeline::JDBCScanLocalState*
parent, int64_t limit,
const TupleId& tuple_id, const std::string& query_string,
TOdbcTableType::type table_type, bool is_tvf, RuntimeProfile*
profile);
- Status open(RuntimeState* state) override;
+ Status _open_impl(RuntimeState* state) override;
Review Comment:
`_open_impl()` is exposed as a public method, which lets callers skip
`Scanner::open()` and its timing logic. If the goal is to enforce NVI, move
`_open_impl()` to `protected:` (consistent with `_get_block_impl()`).
##########
be/src/vec/exec/scan/meta_scanner.h:
##########
@@ -54,7 +54,7 @@ class MetaScanner : public Scanner {
const TScanRangeParams& scan_range, int64_t limit,
RuntimeProfile* profile,
TUserIdentity user_identity);
- Status open(RuntimeState* state) override;
+ Status _open_impl(RuntimeState* state) override;
Review Comment:
`_open_impl()` is declared public, which permits bypassing `Scanner::open()`
(and the per-scanner timing added there). To keep the NVI pattern enforced,
consider moving `_open_impl()` into `protected:` like `_get_block_impl()`.
##########
be/src/vec/exec/scan/es_scanner.h:
##########
@@ -54,7 +54,7 @@ class EsScanner : public Scanner {
const std::map<std::string, std::string>& docvalue_context, bool
doc_value_mode,
RuntimeProfile* profile);
- Status open(RuntimeState* state) override;
+ Status _open_impl(RuntimeState* state) override;
Review Comment:
`_open_impl()` is currently in the public API, which allows calling it
directly and bypassing `Scanner::open()` (and its `SCOPED_RAW_TIMER`). To
properly apply the NVI pattern, move `_open_impl()` under `protected:`
(matching `_get_block_impl()`).
##########
be/src/vec/exec/scan/file_scanner.h:
##########
@@ -70,7 +70,7 @@ class FileScanner : public Scanner {
RuntimeProfile* profile, ShardedKVCache* kv_cache,
const std::unordered_map<std::string, int>*
colname_to_slot_id);
- Status open(RuntimeState* state) override;
+ Status _open_impl(RuntimeState* state) override;
Review Comment:
`_open_impl()` is public here, which makes it possible to call scanner
initialization without going through `Scanner::open()` (and its
`SCOPED_RAW_TIMER`). To enforce the NVI pattern, `_open_impl()` should be under
`protected:` like `_get_block_impl()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
