Repository navigation
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Mryange
requested review from
924060929,
englefly and
starocean999
as code owners
October 19, 2026 15:34
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all nine changed paths at head 448cb83. No substantiated new findings and no existing blocking inline comments. The review converged in round 1: both complete-scope reviewers and the separate boundary-risk reviewer returned NO_NEW_VALUABLE_FINDINGS on the same empty inline set; all candidates and risk focuses have evidence-backed dispositions.
The traced flow is Nereids parser/analyzer and coercion -> shared-input recognition -> CaseExpr/Thrift layout -> BE factory, prepare/open/clone -> ColumnView matching -> existing result assembly. The type/layout and NULL/constant/selector/skipped-branch risks were dismissed after checking their upstream and downstream guarantees, including session guards and prepared point-query serialization.
Critical checkpoint conclusions:
- Goal and evidence: eligible opt-in CASE expressions evaluate the common input once and compare literal keys directly. Added FE/BE/regression tests cover the intended behavior; they were inspected, not executed here.
- Scope and reuse: the changes stay within CASE lowering/execution, session gating and tests, reusing ColumnView and result assembly.
- Concurrency and locking: no new threads or locks; new execution state is call-local and layout flags are immutable after construction.
- Lifecycle and initialization: ColumnPtr owners outlive views; output columns preserve source COW ownership; cloning preserves the layout flag. No new static initialization dependency or ownership cycle.
- Configuration: the experimental session flag defaults false, is dynamically read during translation, and uses reflective needForward forwarding. No restart is required for session changes.
- Compatibility: existing CASE Thrift fields carry the new layout. Enable the feature only after all BEs support it, as documented; older BEs cannot execute this layout. Default-off operation retains searched CASE. No storage, linker or libc ABI change.
- Protocol definitions: no thrift/protobuf definitions changed; field-ID compatibility comparison is not applicable.
- Parallel paths: searched CASE, short-circuit evaluation, missing context and unsupported shapes retain their existing layout. Cloud/shared-nothing use the common expression path; constant-folding and point-query sends were checked.
- Conditional semantics: exact numeric/decimal types and STRING/VARCHAR aliases match BE dispatch. NULL keys are excluded; NULL payloads cannot match. First match, eager matching-THEN errors, skipped unmatched branches, empty batches and selector-relative rows are preserved.
- Test coverage: integer widths, DecimalV3 widths/scales, strings, NULL/constant inputs, duplicate keys, branch errors, selectors, complex results and 256/257 boundaries have coverage. Production prepare/open/deep-clone paths were traced statically where direct unit tests bypass them.
- Expected results: all 272 checked-in blocks were checked against the 68 SQL cases and agree across all four setting combinations. No discrepancy found; regression ordering and setup follow repository conventions.
- Observability and errors: child Status errors propagate, VExpr validates output size/type, exception boundaries remain present, and diagnostics identify the layout/type. No additional metric is needed for this local expression path.
- Transactions and persistence: no transaction, visible-version, delete-bitmap, journal, rowset or failover change. The new session field uses existing persistence support.
- Data writes and atomicity: execution constructs fresh result columns and introduces no persistent data-write path.
- FE-BE state: existing has_case_expr and expression children convey the selection; no new BE query option or protocol field is required.
- Performance and memory: bounded O(rows * branches), at most 256 branches, one common input evaluation and a tracked DorisVector selection buffer. Equality-column materialization is removed; runtime speedup was not measured.
- Other checks: BE header layering/budgets/PCH, extern-template pairing and unity-skip coverage passed via the pure-text build-hygiene script. No unrelated change or additional substantiated issue found.
No additional user-provided review focus was supplied. Coverage limits: this review is static-only, as required. No builds, unit/regression execution, sanitizers, runtime rolling-upgrade exercise or benchmarks were run. The checked-in outputs do not validate the final matching loop at runtime, and the PR description reports that final-loop runtime rerun and added unit-test execution remain outstanding. No unresolved code candidate or uncovered changed path remains.
Review converged after 1 round(s).
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28299 ms |
Contributor
TPC-DS: Total hot run time: 151035 ms |
Contributor
ClickBench: Total hot run time: 24.43 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
Mryange
force-pushed
the
opt-simple-case-evaluation
branch
from
October 22, 2026 08:23
f120e0b to
da848be
Compare
Contributor
There was a problem hiding this comment.
Static review of PR #69296 at da848be covered all 11 changed paths and the FE translation → existing TCaseExpr layout → BE prepare/open/clone/execute/result flow. One P2 finding: the new typed lookup test builds its expected values from a different key sequence than the CASE it executes. No production correctness defect was substantiated. No existing inline findings require carry-forward; there was no additional user focus.
Critical checkpoint conclusions:
- Goal: eligible CASE expressions evaluate one common input and use linear matching or a fragment lookup; the added FE, BE and SQL tests cover this behavior, but the typed-test oracle needs correction.
- Scope and clarity: changes are focused on CASE translation/execution and tests; constant-result broadcasting also deliberately affects searched CASE.
- Concurrency and locking: no new threads or locks; fragment lookup state is immutable after open, context clones share ownership, and selections/results are local to execution.
- Lifecycle and initialization: child literals own borrowed string-key storage; cloned contexts retain the expression tree and shared fragment state. No new static initialization dependency or ownership cycle was found.
- Configuration: enable_experimental_simple_case defaults to false, is observed during translation, and needForward uses the existing reflective session forwarding path; no restart is needed for new plans.
- Compatibility: no storage-format or function-symbol change. The simple CASE expression layout requires upgraded BEs; the disabled default and explicit operator opt-in are the stated rolling-upgrade constraint. Short-circuit evaluation retains searched layout.
- Thrift/protobuf IDs: no gensrc definitions or IDs changed.
- Parallel paths: disabled/missing context, unsupported types/shapes/coercion, short-circuit, small/constant inputs, and large lookup cases were traced; shared result merging was checked for both searched and simple CASE.
- Conditions and errors: eligibility and threshold branches have concrete constraints/comments; NULL keys are rejected, first-match selection is preserved, matching duplicate THENs still evaluate the entire batch, unmatched THENs are skipped, and child errors propagate.
- Test coverage: tests include NULL payloads, constants, empty batches, native widths, DecimalV3 widths/scales, string keys, duplicate keys, nullable/complex results, selectors, cloned contexts, wide indices, threshold boundaries and branch errors. The inline finding explains why a correct implementation fails the newly added typed suite.
- Expected results: all 39,300 lookup golden rows match an independent fixture calculation. All 272 decimal output blocks (1,604 rows) match independent fixture calculations and agree across the 68 corresponding mode groups; no golden-output error was found.
- Observability: existing expression errors/type checks and the enhanced debug string are sufficient for this local execution optimization; no distributed operation or new metric requirement was identified.
- Transactions and persistence: unchanged; no EditLog/replay, visible-version, delete-bitmap or failover path is involved.
- Writes and atomicity: no data-write or transaction-boundary change is introduced.
- FE/BE propagation: existing TCaseExpr flags and child ordering carry the layout, and BE cloning preserves has_case_expr; no new query-option field needs parallel send-path updates.
- Performance and memory: lookup avoids per-key condition columns; constants avoid full-column expansion. Thresholds retain linear matching for small cases. Lookup storage and matching buffers use Doris allocators; batch result merging retains its per-populated-branch scans. Performance claims were not benchmarked during review.
- Further sweep: decimal/hash equality for eligible inputs, const/nullable storage, NaN/infinity/signed-zero result stores, uint16 branch limits and string lifetimes were rechecked; no additional verified issue remains.
Validation limit: this invocation performed static analysis and review-only fixture calculations. Builds, Doris unit/regression execution and source edits were prohibited and were not performed; the PR description also states its final implementation changes have not been rebuilt or retested. Golden-file generation provenance was not independently verified from the static artifacts.
Review converged after 2 round(s).
Contributor
|
Reviewed the latest head,
My verification here was static code tracing and independent fixture calculations; I did not build or execute the final Doris implementation. |
HappenLee
reviewed
Oct 22, 2026
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed PR #69296 at head b493c2b83230ba74ebdd2361c5022b00d7d1bed7. No new verified findings. All 11 changed files and their relevant upstream/downstream paths were covered. Review converged after 2 rounds, including a final pass over this unchanged review payload.
Existing review context: the wide-decimal hash concern is addressed by the decimal HashCRC32 selection and repeated-limb cases. The duplicate-key test oracle now derives the first match from the actual remapped keys. The native-128 hash follow-up remains known infrastructure context; those inputs are excluded by current FE eligibility. No existing P0/P1 finding was independently confirmed as applicable, and no duplicate comments are added. The focus file supplied no additional concerns.
Critical checkpoint conclusions:
- Goal and evidence: the opt-in lowering reconstructs a common integral, DecimalV3, or string input, evaluates it once, and uses linear/hash matching. FE/BE unit sources and dedicated regression/explain cases cover activation and result semantics; execution was not performed.
- Scope and reuse: changes are focused on CASE translation/evaluation, its session flag, and tests. They reuse ColumnView, FunctionContext, Doris hashes/allocators, and existing result assembly.
- Concurrency and locks: fragment state is initialized before worker context cloning and read immutably during execution; selection buffers are per call. No new threads, locks, or lock-order dependency were introduced.
- Lifecycle and initialization: cloned contexts retain the shared expression root and fragment-state owner, preserving borrowed literal string bytes. Independent expression clones rebuild state. RAII handles partial initialization; no new cross-translation-unit static dependency was found.
- Configuration:
enable_experimental_simple_casedefaults to false, uses normal session forwarding/persistence, and is consulted during plan translation without a restart. Short-circuit evaluation takes precedence. - Compatibility: existing TCaseExpr fields are reused; no storage-format change. The documented prerequisite is to upgrade every BE before enabling the flag. Enabled layouts are not supported on old BEs; capability negotiation is not added.
- Thrift/protobuf IDs: no gensrc definitions changed; cross-branch field-ID comparison is not applicable.
- Parallel paths: searched CASE and short-circuit fallback retain their layouts; linear/hash matching and shared result assembly were checked. Distributed plans, point queries, and constant folding use the common serializer; constant CASE folding cannot select this slot-dependent lowering.
- Conditions and errors: eligibility preserves numeric/decimal types and scale, input identity, and non-NULL literal keys. Branch limits, empty/NULL/constant inputs, selectors, first-match results, shadowed duplicate THEN evaluation, and child error propagation were checked against the surrounding APIs.
- Test coverage: inspected threshold boundaries, 256/257 and maximum branch counts, nullable payloads, constant/full/mixed columns, cloned contexts, unmatched and failing branches, string edge keys, decimal widths, complex results, coercion fallbacks, and flag combinations. Dedicated single-CASE explain checks account for CSE potentially bypassing lowering in multi-CASE projections.
- New expected results: independently derived and matched all 150 lookup count/checksum sections and all 272 decimal output sections (68 canonical cases). This was a static arithmetic/string oracle, not Doris test execution.
- Observability: existing CASE rendering distinguishes the selected layout, debug output includes its flags, and Status/type/row-count checks cover failures. No additional production metric need was identified for this local expression change.
- Transactions and persistence: transaction logs, committed-data visibility, partition versions, delete bitmaps, and failover behavior are unchanged. The new session field follows existing annotated serialization.
- Data writes and atomicity: production write paths are unchanged; no new transaction/atomicity interaction was identified.
- FE/BE variable propagation: the flag is an FE plan choice; existing
has_case_exprand child layout carry the result through the common serializer, with no new runtime option requiring separate send-path updates. - Performance and memory: lookup tables and matching buffers use Doris allocator paths, and constant numeric THEN columns avoid full materialization. Hash matching has expected linear batch cost; numeric result assembly retains its existing branch-by-row cost. No measured speedup or runtime memory-limit result is claimed.
- Other checks: reviewed includes, branch-index widths, dispatch agreement, clone/close behavior, and result nullability normalization. No additional substantiated issue or unresolved concrete candidate remains.
Limits: static-only review as required. No builds, Doris tests/SQL, sanitizers, release-artifact ABI checks, performance benchmarks, or mixed-version cluster runs were performed. The historical generation of the .out files cannot be proved from static artifacts. No changed-file coverage gap remains within that scope.
Review converged after 2 round(s).
Contributor
|
run buildall |
HappenLee
previously approved these changes
Oct 26, 2026
HappenLee
left a comment
HappenLee
left a comment
Contributor
There was a problem hiding this comment.
Reviewed head b493c2b83230ba74ebdd2361c5022b00d7d1bed7. No additional blocking correctness issue found in the FE translation, BE matching/result assembly, or context lifetime/clone paths. The wide-decimal hash selection and duplicate-key oracle findings are addressed in this head.
I also extracted the actual shared numeric result-assembly function and checked it against per-row selection under UBSan for full/constant/mixed columns, vector tails, NaN/Inf/signed-zero, and uint8/uint16 branch-index boundaries; that isolated probe passed. This is not a substitute for Doris unit/regression execution. I have triggered run buildall for this exact head; merge must wait for the required latest-head checks.
The remaining per-populated-result-column scan cost is an existing result-assembly strategy and a performance follow-up, not a newly demonstrated correctness regression. No final-head SQL speedup or complete Doris test success is claimed here. Native-128 IN/NOT IN hashing is outside this PR's SQL activation scope.
Review checkpoint conclusions
- Goal: eligible CASE inputs are evaluated once with linear/hash matching; first-match selection is preserved. The previously reported hash and test-oracle defects are fixed.
- Scope/reuse: changes remain in CASE translation/execution and tests; ColumnView, FunctionContext, Doris allocators and existing HashCRC32 are reused.
- Concurrency: fragment state is initialized before context cloning and read immutably; batch selections are local. No new threads or locks.
- Lifecycle: clones share fragment-state ownership and expression roots; literal children retain borrowed string storage. No ownership cycle or new static initialization dependency found.
- Configuration: default-off, forwarded session flag; short-circuit evaluation retains searched CASE.
- Compatibility: existing expression fields are reused. All BEs must be upgraded before enabling the experimental flag, as documented. No storage-format change.
- Protocol IDs: no Thrift/protobuf definitions changed.
- Parallel paths: searched/short-circuit fallback, linear/lookup matching, NULL/constant/selector inputs and nullable/complex results were checked. Shared constant-result broadcasting also affects the disabled-flag path and is included in the required CI coverage.
- Conditions/errors: comparison type/scale, common-input identity and literal eligibility are guarded; branch-index limits reserve zero correctly; duplicate matching THENs retain batch evaluation and child error propagation.
- Coverage: supplied tests cover thresholds, first matches, duplicates, NULLs, vector tails, wide indices, clones and fallback modes. The isolated result-assembly probe and oracle calculation passed; final Doris execution is pending CI.
- Expected results: the corrected oracle follows the same last-key remapping as CASE construction. No expected-result files were edited during this review; generation/execution provenance was not re-established locally.
- Observability: existing layout/debug output and Status/type checks are sufficient for this expression change. Repeated-limb correctness tests alone do not measure hash quality; collision counts/performance measurements remain useful follow-up evidence.
- Transactions/persistence: no transaction, visible-version, delete-bitmap, EditLog or failover change.
- Writes/atomicity: no persistent data-write path is changed; local result ownership and existing exception boundaries remain intact.
- FE/BE propagation: central expression serialization, layout flags, child ordering and clone paths carry the selected layout; unsupported inputs fall back.
- Performance/memory: matching removes equality-column materialization; literal lookup uses tracked containers and fixed-width selection buffers. Numeric result assembly can still scan rows per populated result column; further gather/SIMD comparisons require measurement.
- Additional review: no other substantiated blocking finding. Approval is for this exact reviewed head; ordinary branch protection and required build/test checks still apply.
…matching ### What problem does this PR solve? Issue Number: N/A Problem Summary: Preserve eligible CASE expressions with a common input and literal keys behind a disabled-by-default experimental setting. Compare keys directly in BE, retain first-match and batch-level THEN evaluation semantics, and allow numeric matching loops to auto-vectorize. Short-circuit evaluation continues using searched CASE. Enable only after all BEs support the expression layout. ### Release note Add the opt-in enable_experimental_simple_case setting, disabled by default. ### Check List (For Author) - Test: BE/FE builds and targeted regression generation/rerun passed before the final loop simplification. Final actual-source optimized diagnostic compilation and SIMD reports verified; full rebuild and regression not rerun after the loop change. Unit tests added but not executed. Formatting, hygiene and targeted clang-tidy checked. - Behavior changed: Yes, only when the experimental setting is enabled and short-circuit evaluation is disabled. - Does this need documentation: No, experimental setting disabled by default.
### What problem does this PR solve? Problem Summary: Matching each literal WHEN key scans the input batch repeatedly. Build a fragment-local lookup table for numeric and string keys above a branch threshold, preserve first-match results and batch evaluation of duplicate keys, and avoid expanding constant THEN columns. Extend branch indices beyond the former 256-branch limit. An earlier INT/BIGINT/STRING implementation measured 256-branch mixed SQL speedups of 1.23x, 1.97x and 4.61x over linear matching on a shared machine. These measurements predate the final type, duplicate-key and allocator changes and are not validation of the final patch. ### Release note Improve matching of eligible simple CASE expressions with many literal WHEN branches. ### Check List (For Author) - Test: Earlier implementation passed BE UT (104), FE UT (11), related regressions and RELEASE builds; final changes have only clang-format, build hygiene and diff checks and have not been rebuilt or retested. - Behavior changed: Yes; extend eligible simple CASE branch counts and internal numeric execution coverage. - Does this need documentation: No
### What problem does this PR solve? Problem Summary: The lookup regression records every input row for each combination, producing almost 40000 output lines. Aggregate row counts, non-NULL result counts and checksums of row IDs and results to retain coverage with 451 output lines. ### Release note None ### Check List (For Author) - Test: Generated output using run-regression-test.sh -d correctness_p0 -s test_simple_case_lookup -forceGenOut and reran normally; both passed against the existing 448cb83 service. All 75 setting pairs match. This does not validate the latest BE source. - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Problem Summary: The default Decimal128 and Decimal256 hashes XOR independently hashed limbs. Distinct keys with repeated limbs therefore collide, making lookup initialization quadratic and missed probes linear in the number of WHEN branches. Use the existing HashCRC32 specializations for decimal keys and add repeated-limb Decimal128/256 cases with 256 and 1024 branches. Also correct the test oracle for the final duplicate key. ### Release note Avoid pathological decimal-key hash collisions in experimental simple CASE lookup. ### Check List (For Author) - Test: clang-format 16, build hygiene and git diff --check passed. Added unit tests have not been compiled or run. - Behavior changed: No; matching results are unchanged. - Does this need documentation: No
Mryange
force-pushed
the
opt-simple-case-evaluation
branch
from
October 26, 2026 12:33
b493c2b to
b694e0d
Compare
Contributor
Author
|
run buildall |
### What problem does this PR solve? Related PR: apache#69296 Problem Summary: Including VExprContext in the simple CASE unit test raises the transitive reach of util/rle_encoding.h to 909 translation units, exceeding the build hygiene limit of 908 and failing CMake configuration. Extract ColumnIteratorOptions unchanged into a dedicated header and include it from VExprContext instead of column_reader.h. This reduces the RLE header reach to 272 translation units without increasing the budget. ### Release note None ### Check List (For Author) - Test: clang-format 16, build-support/check-build-hygiene.sh and staged diff checks passed. Compilation and runtime tests not run; this change only relocates an unchanged options structure. - Behavior changed: No - Does this need documentation: No
Mryange
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
October 27, 2026 03:23
Contributor
Author
|
run buildall |
### What problem does this PR solve? Related PR: apache#69296 Problem Summary: Decoupling VExprContext from column_reader.h exposed callers that relied on its transitive includes. QueryContext uses a nested BlockFileCache type without its definition, and scanner, file reader and datetime code similarly require explicit type dependencies. Add the required direct includes and a cache forward declaration without changing execution logic or build hygiene budgets. ### Release note None ### Check List (For Author) - Test: ASAN BE build passed with ./build.sh --be -j48 (exit 0), including linking and packaging. clang-format 16, build hygiene and staged diff checks passed. Unit and regression tests not run. - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28779 ms |
Contributor
TPC-DS: Total hot run time: 150964 ms |
Contributor
ClickBench: Total hot run time: 24.23 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Related PR: apache#69296 Problem Summary: Removing transitive storage dependencies from VExprContext exposed missing type definitions and a leaked RowRanges alias in unit tests. Add direct includes and use the existing native reader alias. Fix the simple CASE typed test's unused lambda capture and construct DecimalV2 fields with the required value type so all BE unit test sources compile. ### Release note None ### Check List (For Author) - Test: ASAN BEUT compilation and linking passed via run-be-ut.sh. CASE, index and Hudi tests passed (139 tests); ParquetV2NativeDecoderTest passed (119 tests), for 258 passing tests total. clang-format 16, build hygiene and diff checks passed. - Behavior changed: No - Does this need documentation: No
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
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: N/A
Problem Summary:
Nereids translates simple CASE expressions into searched CASE, materializing a separate equality result for each WHEN key. This PR adds two optimizations behind
enable_experimental_simple_case(disabled by default):Preserve first-match results and batch-level THEN evaluation, including duplicate keys. FE eligibility remains restricted to compatible integer, DecimalV3 and string comparisons with non-NULL literal keys; unsupported expression shapes and short-circuit evaluation retain searched CASE. Remove the former 256-branch cap in favor of the BE branch-index capacity; the general FE expression-size limit still applies.
Real SQL measurements comparing lookup with linear matching, with the same constant-THEN optimization in both variants, show benefits for many branches. For 256 branches with mixed hits and misses, measured SQL speedups were 1.23x for INT, 1.97x for BIGINT and 4.61x for STRING. These are shared-machine results from an earlier implementation: machine load varied, some smaller cases had higher SQL times, and the final type, duplicate-key and allocator changes have not been rebenchmarked.
Enable the experimental setting only after all BEs have been upgraded: older BEs do not support this expression layout.
Release note
Add opt-in simple CASE execution with direct literal matching and hash lookup for expressions with many WHEN branches. The setting is disabled by default and applies only when short-circuit evaluation is disabled.
Check List (For Author)
Test
An earlier implementation passed 104 BE unit tests, 11 FE unit tests, five related regression suites, RELEASE BE/FE builds, and targeted clang-tidy/checkstyle checks. Regression output was generated by the harness and rerun normally. Lookup-versus-linear SQL validation covered 45 workloads over four rounds (1,800 measured queries), with matching results and normalized plans, one scanner, one pipeline instance, default batch size 8160 and caches disabled. Numeric matching and result-merge SIMD was confirmed in compiler records and the built binary; hash probing itself does not auto-vectorize.
The final numeric-type extension, duplicate-key handling, helper extraction and Doris allocator changes have only formatting, build hygiene and diff checks; they have not been rebuilt or retested.
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)