THRIFT-6149: Avoid decoding skipped Ruby strings - #3713
Merged
Conversation
Client: rb Co-Authored-By: OpenAI Codex (GPT-5.6) <codex@openai.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes Ruby’s protocol skip(Thrift::Types::STRING) path so Binary and Compact protocols can consume skipped STRING values without constructing/validating UTF-8 strings, while preserving the existing behavior for JSON and the base protocol.
Changes:
- Introduces
BaseProtocol#skip_stringand routesBaseProtocol#skip(... STRING ...)through it. - Implements protocol-specific
skip_stringbehavior (Binary/Compact viaread_binary, JSON viaread_string), and forwards through Header/Decorator protocols. - Adds Ruby specs validating the new delegation and skip behavior (including decorator/wrapper coverage).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/rb/lib/thrift/protocol/base_protocol.rb | Adds skip_string hook and updates skip(STRING) to call it |
| lib/rb/lib/thrift/protocol/binary_protocol.rb | Implements skip_string using read_binary to avoid UTF-8 conversion |
| lib/rb/lib/thrift/protocol/compact_protocol.rb | Implements skip_string using read_binary to avoid UTF-8 conversion |
| lib/rb/lib/thrift/protocol/json_protocol.rb | Implements skip_string via read_string to preserve JSON semantics |
| lib/rb/lib/thrift/protocol/header_protocol.rb | Delegates skip_string to the selected underlying protocol |
| lib/rb/lib/thrift/protocol/protocol_decorator.rb | Forwards skip_string to the decorated protocol |
| lib/rb/spec/binary_protocol_spec_shared.rb | Tests that skipping STRING uses read_binary (not read_string) |
| lib/rb/spec/compact_protocol_spec.rb | Tests that skipping STRING uses read_binary (not read_string) |
| lib/rb/spec/json_protocol_spec.rb | Tests skipping unknown JSON string fields avoids base64 decoding and works through decorators |
| lib/rb/spec/header_protocol_spec.rb | Tests HeaderProtocol delegates skip_string to its selected protocol |
| lib/rb/spec/protocol_decorator_spec.rb | Tests ProtocolDecorator forwards skipped strings to the decorated protocol |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Ruby clients and servers skip fields they do not recognize when communicating across schema versions. The shared Ruby protocol path currently handles skipped
STRINGvalues throughread_string. Binary and Compact protocols therefore construct a UTF-8 string from the raw bytes even though the value is immediately discarded.This change gives protocols control over how skipped strings are consumed. Binary and Compact use their binary readers, while the base protocol preserves the existing string behavior for custom protocols. JSON continues reading skipped values as strings because JSON strings and binary values have different wire representations. Header and decorated protocols forward the operation to their selected or wrapped protocol.
The wire bytes consumed are unchanged. The change only avoids unnecessary representation work when Binary or Compact string values are skipped.
Benchmarks
The benchmark used Ruby 4.0.6 on aarch64 Linux and skipped 100,000 strings of 128 bytes each. Each result is the median of five measured trials after one warm-up trial. The same temporary harness and configuration were run against master commit
9b10484ca7687af6cc67567df9d457d0f031748eand the proposed change:JSON must retain its string-reading path. A separate alternating paired benchmark of 20,000 skipped JSON strings measured the forwarding overhead at +0.53% in native mode and +0.06% in pure Ruby, with unchanged allocations.
These are synthetic skip-only workloads on one Ruby version and architecture. The temporary harness is not included in the change.
[skip ci]anywhere in the commit message to free up build resources.