[fix](expr) Canonicalize logical OR results - #68308
Merged
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes — capped/incomplete static review (round 3 of 3)
I reviewed the authoritative five-file diff at exact head e358d870510ae9fc6fc73a98a078cdc73f952342. Four blocking findings are attached inline: the compound unit test cannot reach its intended kernel, OR identity/direct consumers remain unsafe for legacy noncanonical Booleans, the OR null-mask formulas still test the low bit rather than truthiness, and logical XOR remains an independently wrong mixed-version consumer. A new valuable issue surfaced in the third and final permitted round, so this review is explicitly capped/incomplete; the workflow forbids a fourth convergence round.
Critical checkpoint conclusions:
- Goal/correctness: Rows that enter the patched OR data loops are canonicalized, and canonical
TRUE/FALSE/NULLinputs retain the right SQL truth table. The end-to-end goal is not complete because the four inline cases remain. - Scope/clarity: The diff is small and its local intent is clear, but it does not cover the identity returns, null-mask calculation, or parallel logical consumers required by the same Boolean invariant.
- Concurrency/thread safety: The production changes are stateless row-local arithmetic. They introduce no shared state, locks, threads, or deadlock surface.
- Lifecycle/ownership: Production ownership is unchanged. The new test has a concrete lifecycle/dispatch defect: its childless fake expression is classified constant and enters an unprepared generic fallback.
- Configuration: No configuration item or dynamic-update path is added.
- Compatibility: Existing nullable/numeric PBlock serialization preserves raw Boolean bytes across BE exchange. Mixed-version execution is therefore applicable and is not safely handled by OR-only normalization; the identity, CASE, null-mask, and XOR cases are blockers.
- Parallel paths and conditions: Generic/compound, nullable/non-nullable, constants, selectors, index materialization, identity returns, regular/short-circuit CASE, and both operand orders were traced. Constants use the generic path and index results are canonical; the unsafe conditions are identified inline.
- Test coverage/results: BE test discovery is valid, and the two direct generic tests exercise their intended paths. The compound test does not. The regression labels and outputs are internally consistent: 34,000 true rows, 66,000 NULL rows, 100,000 non-NULL CASE results, and the nine-row truth table all recompute correctly. No test covers the mixed-version identity, even-truthy null-mask, direct CASE, or XOR shapes.
- Observability: No new long-running or operational path needs logs or metrics.
- Transactions/persistence/data writes: No transaction, edit-log, storage-format, visibility, MoW, or atomic-write behavior changes.
- FE-BE variables/protocol fields: No new variable or wire field is introduced; the relevant existing PBlock value compatibility is the failing concern above.
- Performance: Work remains O(rows), vector-friendly, and allocation-free in the changed loops; no material CPU or memory regression was found.
- Other: The PR describes a user-visible behavior fix but leaves the release note as
Noneand the behavior/test checklist unchecked; please update the metadata to match the actual change and validation. No additional user review focus was supplied.
Validation is static only by task restriction: I did not build or run unit/regression tests, and CI/author claims are not treated as independent execution evidence.
| class ColumnExpr final : public VExpr { | ||
| public: | ||
| ColumnExpr(ColumnPtr column, DataTypePtr type) | ||
| : VExpr(std::move(type), false), _column(std::move(column)) {} |
Contributor
There was a problem hiding this comment.
[P1] Keep this synthetic column expression off the constant path. Because it has no children, inherited VExpr::is_constant() returns true, so VCompoundPred::_has_const_child() delegates this test to VectorizedFnCall instead of reaching do_null_pred<false>. This helper never prepares/opens the expression, leaving _function unset; the fallback fails in _do_execute(), and the test provides no coverage for the compound change.
Suggested change
| : VExpr(std::move(type), false), _column(std::move(column)) {} | |
| : VExpr(std::move(type), false), _column(std::move(column)) {} | |
| bool is_constant() const override { return false; } |
| } else { | ||
| lhs[i] |= rhs[i]; | ||
| // Logical OR must produce a canonical Boolean instead of preserving input bits. | ||
| lhs[i] = (lhs[i] != 0) || (rhs[i] != 0); |
Contributor
There was a problem hiding this comment.
[P1] Preserve compatibility for OR results that bypass these loops. The block-wide shortcuts at lines 521-536 return an operand unchanged, so during a rolling upgrade an old BE can emit byte 65 for NULL(payload=65) OR TRUE, PBlock exchange preserves it, and a new BE can pass 65 into regular CASE branch indexing. Canonicalizing those returned operands fixes this route, but an exchanged legacy OR result can also feed CASE directly; please additionally interpret CASE conditions by truthiness (or canonicalize at a defined compatibility boundary) and cover the mixed-version shape.
| static inline constexpr ResultType apply(UInt8 a, UInt8 b) { return a | b; } | ||
| // A NULL row may carry an arbitrary nested byte. Canonicalize logical OR to 0 or 1 so that | ||
| // such a byte cannot become visible when the other operand makes the result non-NULL. | ||
| static inline constexpr ResultType apply(UInt8 a, UInt8 b) { return (a != 0) || (b != 0); } |
Contributor
There was a problem hiding this comment.
[P1] Normalize the null decision as well as the data byte. The unchanged apply_null below tests the low bit via 1 ^ a, not truthiness. A supported rolling-upgrade path can produce an even true byte: old OR emits {65,1}, XOR with TRUE yields {64,0}, and PBlock preserves it; on the new BE, 64 OR NULL reaches this formula and is marked NULL instead of TRUE. Please derive nullness with a == 0 / b == 0 in both OrImpl and VCompoundPred, and test both operand orders.
| static inline constexpr ResultType apply(UInt8 a, UInt8 b) { return a | b; } | ||
| // A NULL row may carry an arbitrary nested byte. Canonicalize logical OR to 0 or 1 so that | ||
| // such a byte cannot become visible when the other operand makes the result non-NULL. | ||
| static inline constexpr ResultType apply(UInt8 a, UInt8 b) { return (a != 0) || (b != 0); } |
Contributor
There was a problem hiding this comment.
[P1] This normalization leaves another rolling-upgrade consumer incorrect. An old BE can send the semantically true byte 65 produced by pre-fix OR; PBlock preserves it, and the new BE's registered XorImpl still computes 65 ^ 1 = 64. Doris interprets 64 as true, so TRUE XOR TRUE returns TRUE before any later OR or CASE. Please normalize XOR by operand truthiness ((a != 0) != (b != 0)) or define one Boolean canonicalization boundary, and add a mixed-version case.
Contributor
TPC-H: Total hot run time: 27969 ms |
Contributor
TPC-DS: Total hot run time: 151992 ms |
Contributor
ClickBench: Total hot run time: 24.04 s |
Contributor
Author
|
/review 重新review一下代码。请注意，我这是一个fix，不需要考虑升降级兼容性。我们为什么要兼容错误的行为呢？ |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27676 ms |
Contributor
TPC-DS: Total hot run time: 152257 ms |
Contributor
ClickBench: Total hot run time: 24.03 s |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
run external |
HappenLee
reviewed
Sep 21, 2026
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27826 ms |
Contributor
TPC-DS: Total hot run time: 152029 ms |
Contributor
ClickBench: Total hot run time: 23.79 s |
16 tasks
morningman
added a commit
that referenced
this pull request
Sep 22, 2026
… the query; stop the regression framework leaking connections (#68338) ### What problem does this PR solve? Related PR: #68101 (the one connection pool, which made the leak visible), #68266 (the bearer token as the session's credential, which removed its last cap), #67503 / #62259 (the Arrow Flight deferral gate this generalizes) Problem Summary: **Context.** A Remote Doris catalog with `use_arrow_flight = true` reads another Doris cluster over Arrow Flight SQL: for every scan, `RemoteDorisScanNode` on the local FE performs a Flight SQL handshake against a remote FE (`authenticateBasicToken`), runs the query there (`GetFlightInfo`), and hands the endpoints - a ticket per remote BE - to the local BE, which reads the rows with `DoGet` straight from the remote BEs. The handshake is not free on the remote side: since #68101 a Flight SQL session is a connection in the remote FE's one connection pool, counted against `qe_max_connection`, the Arrow Flight SQL sub-quota and the catalog user's `max_user_connections`; since #68266 the bearer token is that session's name in the pool and nothing else, so the session ends only on `CloseSession`, `KILL CONNECTION`, `wait_timeout` (8h by default) or an FE restart. The regression framework has a leak of its own of the same shape: a suite's Doris connections are `ThreadLocal` to the thread that opened them, and only the suite thread and `Suite.thread()` close theirs; a `sql` on any other thread opens a connection nobody closes. **1. The problem, and what it cost** - `RemoteDorisScanNode.executeFlightSqlQuery` closed the gRPC channel and the allocator in a try-with-resources but never sent `CloseSession`. Every scan of a Remote Doris table therefore left one Flight SQL session behind on the remote FE, under the catalog user, until `wait_timeout`. Before #68101 this was invisible: Flight sessions had a pool of their own, were not counted per user, and a per-user LRU of `max_user_connections / 2` tokens evicted the oldest. After #68101 the leaked sessions eat the catalog user's quota on the remote FE; after #68266 nothing caps them at all. A hundred scans within 8h and the user - MySQL clients included - is refused there with `Reach limit of connections`. - This is what broke the external regression pipeline on 2026-09-21 (TeamCity 1053416 on #68308): the `remote_doris` suites point the catalog at the FE under test with user `root`; 49 scans left 48 Flight sessions (`Arrow Flight SQL: 512 (current: 48)` in the refusal), which took half of root's 100; the other half was taken by the framework leak below, and 14 MySQL connections were refused in a six-second window. - The framework leak: `Awaitility.await()...until { sql ... }` evaluates the condition on Awaitility's own thread, which dies with the `await()`. Each call leaked one root connection until the client JVM garbage-collected it (the FE logs those as `No more data to be read. Close connection`). 230 suites call `Awaitility.await()` directly; in the failing run one suite opened 22 such connections in 46 seconds, and 29 of them were collected in one GC at the moment the refusals stopped. Suites that call `sql` from threads of their own (`Thread.start { streamLoad }`, an `Executors` pool) leak the same way - a P0 run of the same day shows 76 Awaitility connections and 89 own-thread connections opened by root within one minute, all left to the garbage collector - and the two docker helpers dropped the connection their action opened without closing it. **2. What this PR does, and why it helps** FE: - `RemoteDorisFlightSession` (new): the session as an object - handshake, `execute`, and `close()` = `CloseSession` (bounded to 5s so a remote FE that stopped answering cannot hang the local query's teardown; the session is then left to its `wait_timeout` as before) followed by the channel and the allocator. `open()` leaves nothing behind when the handshake is refused; a query that fails closes its session at once, so the retry on the next node leaves nothing behind either. Idempotent. - `RemoteDorisScanNode` keeps the session from `getSplits` until `stop()`, which the coordinator calls when the local query closes or is cancelled - i.e. when the local BE is done with the remote query's endpoints. It cannot be closed right after `GetFlightInfo`: the remote FE cancels whatever a closed session was still running, and when the remote table is itself an external table scanned in batch mode the remote query is deferred there and still running while the local BE reads. - For the same reason the local coordinator has to outlive dispatch when the local query is itself an Arrow Flight SQL query (otherwise #67503 closes it right after `exec()`, while the local BE may still be reading). The deferral gate's predicate is generalized from "has a batch split source" to "the BE still depends on this scan after dispatch": `ScanNode.hasBatchSplitSource()` -> `coordinatorMustOutliveDispatch()`, `Coordinator.hasBatchSplitSource()` -> `mustOutliveDispatch()`; `RemoteDorisScanNode` adds its open session as the second reason. Batch-mode external scans behave exactly as before. - The statement is the fallback owner. The session is opened while the plan is translated, before any coordinator exists, and not every plan gets a coordinator or gets one that is closed: a statement that fails between planning and dispatch (a SQL block rule on the scan, an `INSERT` whose transaction cannot begin - a re-used `WITH LABEL`, the per-db txn limit), the plan `INSERT OVERWRITE` and every materialized-view refresh run only to locate the sink and discard, a load job created from the plan. `keepFlightSession` registers the node with the `StatementContext`, whose `close()` (the per-statement finally of `ConnectProcessor`, `TaskProcessor`, `MTMVTask`) stops what is still registered; the deferral gate hands the nodes over to the deferred coordinator before `deferForArrowFlight`, so a Flight query kept alive for DoGet is untouched. `INSERT OVERWRITE` releases its probe plan's scan nodes as soon as the plan has been read. - A same-plan retry must not reuse a plan whose scan node released what the BE scans with: `handleQueryWithRetry` re-dispatches the failed attempt's plan after its `cancel()` stopped the scan nodes, and the endpoints of a remote Doris scan belong to the query of a session that is then gone (the remote FE tears down a query it had deferred). `ScanNode.cannotBeRedispatched()` (true for a remote Doris scan once `stop()` ended its session) makes the retry rethrow the original error instead. Regression framework: - `Awaitility.pollInSameThread()` at framework start-up: every `until { }` now runs on the suite thread and reuses the suite's connection, as `Suite.awaitUntil` already did. The trade-off: an `atMost()` no longer bounds a condition that blocks (the poll runs to completion before the bound is checked). A condition that runs statements is bounded by their timeouts; a condition that waits on anything else bounds the wait itself - `SuiteCluster` now runs its `doris-compose` subprocess waits on a helper thread joined with the command's timeout and destroys the process on expiry (they used to rely on `atMost()`). - `SuiteContext` records every connection its thread-local accessors open, with the thread that opened it. On every statement it closes the connections of threads that have finished (a suite that starts a thread per step - `Thread.start { streamLoad }; join`, as the mow flexible suites do 72 times - now holds at most the connections of the threads still running, deterministically, where before the population depended on the JVM's next GC), and when the suite ends it closes whatever is left, with a warning naming the suite. - The two docker helpers (`docker`, `dockers`) close the connection their action opened before restoring the original one (and the multi-cluster one now types that original as the `ConnectionInfo` it is). Tests: - `RemoteDorisScanNodeTest`: an in-process Flight SQL server counts the sessions it is asked to close. The session lives from the query until `stop()`; `stop()` twice closes once; a failed query closes at once; a refused handshake opens nothing; a session handed over after `stop()` is closed at once; the coordinator of a query with such a scan `mustOutliveDispatch()`; a session no coordinator takes ends with the statement, one handed to a deferred coordinator does not; and a node whose `stop()` ended a session `cannotBeRedispatched()`. - `ArrowFlightDeferralGateTest` follows the rename. - Regression `external_table_p0/remote_doris/test_remote_doris_flight_session`: a catalog logging in as a user of its own scans a table five times, then `INSERT OVERWRITE`s from it, then runs an `INSERT ... WITH LABEL` twice (the second is refused after planning), and asserts after each statement that `information_schema.processlist` holds no `ArrowFlightSQL` session of that user - assertions that fail on master. What it buys: a Remote Doris scan costs the remote FE one session for exactly the duration of the local query, whatever the protocol of the local client; the catalog user's quota on the remote FE is no longer consumed by history; and the regression framework no longer manufactures the MySQL half of the pressure. **3. The classes, and how they call each other** - `RemoteDorisScanNode` (existing): `getSplits` -> `executeQuery` -> `executeFlightSqlQuery(host, user, password, sql, timeout)`: `RemoteDorisFlightSession.open` + `execute`, then `keepFlightSession` (which also registers the node with `StatementContext.stopScanNodeAtClose`). `stop()` (from `Coordinator.close()` / `cancel()`, or `StatementContext.close()` as the fallback) closes the session; `coordinatorMustOutliveDispatch()` is true while one is held; `cannotBeRedispatched()` once `stop()` ended one. - `RemoteDorisFlightSession` (new): `open` (FlightClient + `authenticateBasicToken`), `execute` (`FlightSqlClient.execute`), `close` (`closeSession` with a 5s deadline, then client and allocator). - `ScanNode.coordinatorMustOutliveDispatch()` (renamed from `hasBatchSplitSource`): `splitAssignment != null`, overridable. `ScanNode.cannotBeRedispatched()` (new): false by default. - `Coordinator.mustOutliveDispatch()` (renamed): any scan node's `coordinatorMustOutliveDispatch()`. - `StmtExecutor.executeAndSendResult`: the deferral gate now reads `coord.mustOutliveDispatch()`; the deferral gate calls `StatementContext.handOverScanNodesToDeferredCoordinator` before `deferForArrowFlight`; `handleQueryWithRetry` rethrows instead of retrying when `planCannotBeRedispatched()`. - `StatementContext`: `stopScanNodeAtClose`, `handOverScanNodesToDeferredCoordinator`, and `close()` stopping what is left (after the table locks, before the connector scope). - `InsertOverwriteTableCommand.run`: stops the scan nodes of the plan it probes and discards. - `SuiteCluster.waitForDorisCompose`: the bounded subprocess wait `runCmd` / `runCmdList` use instead of `Awaitility.await().atMost(...)`. - Remote FE, untouched: `DorisFlightSqlProducer.closeSession` -> `FlightSessionsInConnectPool.closeConnectContext` -> `ConnectContext.cleanup()` + `cancelQuery`. - `RegressionTest.initGroovyEnv`: `Awaitility.pollInSameThread()`. - `SuiteContext`: `openedDorisConnections` (connection -> opening thread), `trackDorisConnection`, `closeConnectionsOfFinishedThreads` (from `getConnection()`, i.e. every statement), `closeDorisConnection`, `closeLeftoverDorisConnections` (from `close()`); `Suite.dockerImpl` / `dockers` call `closeDorisConnection`. ``` local FE remote FE remote BE RemoteDorisScanNode.getSplits '- executeFlightSqlQuery |- RemoteDorisFlightSession.open ---- handshake --> openSession (pool: +1 for the catalog user) |- session.execute ----------------- GetFlightInfo --> runs the query ---------------> result buffered '- keepFlightSession (ticket per BE) Coordinator.exec -> local BE ------------------------------ DoGet(ticket) --------------------> rows (Arrow Flight local client: coordinator kept alive, mustOutliveDispatch() == true) Coordinator.close / cancel '- scanNode.stop '- session.close ------------------- CloseSession --> closeConnectContext (pool: -1) ```
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68280 Problem Summary: Logical OR used bitwise OR on the nested bytes of Boolean columns. Nullable rows may carry arbitrary nested payloads, so a NULL operand combined with TRUE could expose a non-canonical non-NULL Boolean value instead of 1. Downstream expressions such as CASE could then treat that byte as an invalid branch index. Canonicalize OR results to 0 or 1 in both the generic logical function and VCompoundPred paths. ### Release note Fix logical OR to return canonical Boolean values for nullable inputs. ### Check List (For Author) - Test: Regression test and manual test - Behavior changed: Yes. Logical OR now canonicalizes non-NULL results to 0 or 1. - Does this need documentation: No
### What problem does this PR solve?
Issue Number: N/A
Problem Summary: The synthetic column expression in the logical OR unit test inherited the default constant-expression classification because it has no children. This made VCompoundPred fall back to the generic function path instead of exercising the nullable compound OR kernel. Mark the test expression as non-constant so the test covers the intended implementation.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `./run-be-ut.sh --run --filter='FunctionsLogicalTest.*' -j48`
- Behavior changed: No. Test-only correction.
- Does this need documentation: No
morningman
force-pushed
the
fix-nullable-or-payload
branch
from
September 22, 2026 02:54
3da0332 to
22b2cf5
Compare
Contributor
|
run buildall |
Contributor
TPC-H: Total hot run time: 27408 ms |
Contributor
TPC-DS: Total hot run time: 152816 ms |
Contributor
ClickBench: Total hot run time: 24.01 s |
Contributor
|
/review |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
Request changes: the two modified row loops now canonicalize OR with the focused, SIMD-friendly (a | b) != 0 form, and the new unit/regression cases are statically discoverable, reach the intended registered/compound paths, and have coherent deterministic outputs. Two additional P1 issues still block the fix: the nullable compound path can read a freed synthetic null map, and a valid legacy noncanonical TRUE still breaks nullable non-short-circuit IF.
I did not duplicate the live threads already covering OR shortcut/regular-CASE compatibility, the nullable OR decision formula, or XOR. The test-helper issue is addressed on this head, and the packed-expression performance concern is resolved.
Checkpoint conclusions: the change is focused and the primary reproduction is covered, but the overall rolling-compatible Boolean goal is incomplete. Registered/compound, constant/vector, nullable/non-nullable, shortcut, planned-versus-physical nullability, CASE/IF, parallel logical operators, and filter consumers were traced. No new concurrency, locking, initialization-order, configuration, transaction/EditLog, durable-format, FE-BE-field, data-write, ABI, observability, or error-propagation concern was found. Raw PBlock Boolean preservation remains the relevant compatibility boundary. The changed tests and expected results are statically sound, but neither blocking case below is covered. This was a static-only review as required: I did not build or run tests, so author/CI test claims are not independent reviewer validation. No additional user focus was supplied.
| res_data[i] = lhs_data[i] | rhs_data[i]; | ||
| // A NULL row may carry an arbitrary nested byte. If the result remains NULL the | ||
| // byte is ignored; otherwise normalization prevents it from becoming visible. | ||
| res_data[i] = (lhs_data[i] | rhs_data[i]) != 0; |
Contributor
There was a problem hiding this comment.
[P1] Keep both synthesized null maps alive here. A planned Nullable(Boolean) child may legally return a physical non-nullable ColumnUInt8 for an all-non-null batch (VExpr explicitly permits this). If both children do so with mixed values, no shortcut fires and vector_vector_null calls create_null_map_column twice through the same temp_null_map owner. The second assignment destroys the first zero map, leaving lhs_null_map_tmp dangling before do_null_pred reads it in this loop. Under ASAN this is a use-after-free; otherwise result nullness comes from freed memory. Please retain two owners or deliberately share one live zero map, and add a planned-nullable case with two mixed physical non-nullable children.
| END) IS NOT NULL | ||
| """ | ||
|
|
||
| order_qt_case_branches_no_short_circuit """ |
Contributor
There was a problem hiding this comment.
[P1] Please cover the nullable IF consumer as part of this compatibility fix. A legacy OR can validly expose mixed condition bytes {65,0} (65 is SQL TRUE), and PBlock preserves them. With a nullable non-NULL THEN value and NULL ELSE, both non-short-circuit IF implementations call apply_negated_null_map; it computes 1 ^ 65 = 64, so the true row becomes NULL instead of returning THEN. This is distinct from the existing regular-CASE thread: a local CASE truthiness fix leaves FunctionIf and VectorizedIfExpr wrong. Canonicalize before using Boolean bytes as null maps (or enforce the boundary earlier) and test this shape with both short-circuit settings.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run vault_p0 |
Mryange
dismissed
github-actions[bot]’s stale review
September 22, 2026 07:13
第一条review意见是他过去就这样，
第二条他要我fix的PR去考虑兼容性？
mrhhsg
approved these changes
Sep 22, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
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
6 participants
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.
Logical OR used bitwise OR on the nested bytes of Boolean columns. Nullable rows may carry arbitrary nested payloads, so
NULL OR TRUEcould expose a non-canonical non-NULL Boolean byte instead of1. Downstream expressions such as multi-branchCASE WHENcould then use that byte as an invalid branch index and crash the BE.Root cause: both the generic logical function and the
VCompoundPredfast path preserved input bits instead of producing a canonical Boolean result. This change normalizes every logical OR result to0or1while preserving SQL three-valued NULL semantics. It also adds focused BE coverage and SQL regressions for nullable payloads,CASE WHEN, short-circuit evaluation settings, and the completeTRUE/FALSE/NULLOR truth table.The resulting nullable Boolean representation follows this truth table.
NULL(any)means that the nested byte of a NULL row may contain any value; it is ignored wheneverres_nullis1.res_nullres_dataFALSEFALSEFALSEFALSETRUETRUETRUEFALSETRUETRUETRUETRUENULL(any)FALSENULLFALSENULL(any)NULLNULL(any)TRUETRUETRUENULL(any)TRUENULL(any)NULL(any)NULLRelease note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)