Rust: skip string/binary fields without heap-allocating

[lbarthon]

Client: rs

skip(TType::String) allocated a Vec the size of the field via read_bytes() and immediately dropped it. Binary and compact now read the length prefix and discard the payload through a 256-byte stack buffer (skip_binary / discard_exact). Negative binary sizes and max_string_size still apply. Custom TInputProtocol impls keep the allocating default.

• Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[lbarthon]

cc @Jens-G when you have a moment 🙏

[Jens-G]

Code review

Found 1 issue:

1. There is no JIRA ticket for this change, and the PR title and the commit subject do not start with THRIFT-NNNN:. The change adds a method to the public TInputProtocol trait and changes how the binary and compact protocols skip string and binary fields, so it is not a trivial one (AGENTS.md says a ticket is "Required for all non-trivial PRs" and "All significant changes need a JIRA ticket.")

thrift/AGENTS.md

Lines 27 to 29 in 7d6be86

	|---|---|---|
	| Significant changes | [Apache JIRA — THRIFT project](https://issues.apache.org/jira/browse/THRIFT) | Required for all non-trivial PRs |
	| Minor / quick fixes | GitHub Issues | Typos, trivial compiler warnings, etc. |

thrift/AGENTS.md

Lines 48 to 50 in 7d6be86

	- One commit per issue (squash before submitting).
	- All significant changes need a JIRA ticket.
	- Provide tests for every submitted change.

One suggestion, below the bar for the list above but verified:

• None of the new tests notices if skip() goes back to read_bytes(), or if the skip_binary forward in Box<P> or in TStoredInputProtocol is dropped, because both paths consume the same bytes and return the same errors. A test transport that records the largest buffer it is asked to fill does notice: while skipping a 1024-byte string it sees a single 1024-byte request on the old path and no request over 256 bytes on the new one. I checked this against all three changes.

thrift/lib/rs/src/protocol/mod.rs

Lines 1213 to 1224 in 7d6be86

	#[test]
	fn must_skip_large_binary_field_through_boxed_protocol() {
	let payload = vec![0xCDu8; 1024];
	let data = build_struct_with_unknown_binary_then_i64(&payload, 9);
	let mut proto: Box<dyn TInputProtocol> =
	Box::new(TBinaryInputProtocol::new(Cursor::new(data), true));
	let (first, second) = skip_unknown_and_read_i64_fields(&mut proto).unwrap();
	assert_eq!(first, 42);
	assert_eq!(second, Some(9));
	}

thrift/lib/rs/src/protocol/mod.rs

Lines 407 to 411 in 7d6be86

	fn skip_binary(&mut self) -> crate::Result<()> {
	(**self).skip_binary()
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}