viirya commented on code in PR #6098:
URL: https://github.com/apache/datafusion-comet/pull/6098#discussion_r4074427428
##########
native/shuffle/src/ipc.rs:
##########
@@ -120,6 +123,29 @@ fn cache_schema(
schema_message: &[u8],
schema: SchemaRef,
) {
+ // Admission only affects reuse. Large valid schemas still decode, without
evicting useful
+ // entries or retaining their serialized and parsed copies for the
lifetime of the thread.
+ if schema_message.len() > SCHEMA_CACHE_ENTRY_RETAIN_LIMIT {
+ return;
+ }
+ let mut retained_size = schema_message
+ .len()
+ .saturating_add(std::mem::size_of_val(schema.as_ref()))
+ .saturating_add(schema.fields().size())
+ .saturating_add(
+ schema
+ .metadata()
+ .capacity()
+ .saturating_mul(std::mem::size_of::<(String, String)>()),
+ );
+ for (key, value) in schema.metadata() {
+ retained_size = retained_size
Review Comment:
Your instinct is right, and I think the current form should stay.
`key.capacity() + value.capacity()` is a plain addition, so it is the one
spot in this computation that could overflow — panicking in a debug build,
wrapping in release. Wrapping is the bad case here: the sum would come out
small, `retained_size` would land under the limit, and an oversized schema
would be admitted, which is exactly the outcome the check exists to prevent.
Reaching it would need two `String` capacities summing past `usize::MAX`, so
on a 64-bit target it is not reachable in practice. But the two chained
`saturating_add`s cost nothing, match the style of the rest of the expression,
and mean the reader does not have to work out that it is unreachable. Folding
them back into one addition would make this the only unguarded arithmetic in
the function.
For what it is worth, I checked the composition itself against arrow-schema
59.3.0 while reviewing: `Fields::size()` recurses through `DataType::size()`
and covers field names and field-level metadata, so this loop is picking up the
schema-level metadata it does not reach. No double counting between the two.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
