Skip to content

refactor: always add condition/if_then branch in TxnRequest conversion#91

Merged
drmingdrmer merged 1 commit intodatabendlabs:mainfrom
drmingdrmer:051-fix-txn
Mar 6, 2026
Merged

refactor: always add condition/if_then branch in TxnRequest conversion#91
drmingdrmer merged 1 commit intodatabendlabs:mainfrom
drmingdrmer:051-fix-txn

Conversation

@drmingdrmer
Copy link
Member

Changelog

refactor: always add condition/if_then branch in TxnRequest conversion

Make the condition/if_then branch unconditionally present when converting
TxnRequest to Transaction. Predicate::and([]) is vacuously true, so an
empty TxnRequest now always matches the "then" path, preserving legacy
behavior while simplifying the executed_branch mapping logic.

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com


  • Improvement

Make the condition/if_then branch unconditionally present when converting
TxnRequest to Transaction. Predicate::and([]) is vacuously true, so an
empty TxnRequest now always matches the "then" path, preserving legacy
behavior while simplifying the executed_branch mapping logic.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 6, 2026 07:18
Copy link
Collaborator

@xp-trumpet xp-trumpet left a comment

Choose a reason for hiding this comment

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

@xp-trumpet reviewed 1 file and all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on drmingdrmer).

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors legacy TxnRequestTransaction conversion to always include the condition/if_then branch, relying on vacuous truth (Predicate::and([])) to preserve legacy semantics for empty requests and simplify executed-branch → reply mapping.

Changes:

  • Always append a condition/if_then branch when converting pb::TxnRequest into internal Transaction.
  • Simplify KvTransactionReply::into_txn_reply() mapping logic by removing conditional “has_if” handling and updating mapping docs.
  • Update/extend tests, including a new case asserting an empty TxnRequest maps to an always-true “then” path.

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

Comment on lines +362 to +364
// Should not happen: the condition/if_then branch is always
// present and Predicate::and([]) is vacuously true,
// so at least that branch always matches.
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

The executed_branch: None arm is documented as “Should not happen” because the condition/if_then branch is always present and Predicate::and([]) is true, but None can still legitimately occur when no branch matches (e.g., non-empty condition evaluates to false and else_then is empty) or when decoding replies produced by older evaluators. Please adjust this comment to reflect the real scenarios where executed_branch may be None and why mapping it to execution_path="else" is the desired backward-compatible behavior.

Suggested change
// Should not happen: the condition/if_then branch is always
// present and Predicate::and([]) is vacuously true,
// so at least that branch always matches.
// This can happen when no branch matches (e.g. a non-empty
// condition evaluates to false and `else_then` is empty), or
// when decoding replies produced by older evaluators that did
// not populate `executed_branch`. In both cases we treat it
// as falling through to the `else` path for backward-
// compatible behavior.

Copilot uses AI. Check for mistakes.
Merged via the queue into databendlabs:main with commit 49c6a5a Mar 6, 2026
7 checks passed
@drmingdrmer drmingdrmer deleted the 051-fix-txn branch March 6, 2026 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants