Repository navigation
Conversation
|
Assigning reviewers: R: @claudevdm for label python. This pull request likely touches a core component ("core" label). Please review with scrutiny. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40501 +/- ##
============================================
+ Coverage 56.16% 56.23% +0.06%
Complexity 2288 2288
============================================
Files 1124 1124
Lines 178336 178366 +30
Branches 1489 1489
============================================
+ Hits 100170 100303 +133
+ Misses 75645 75542 -103
Partials 2521 2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
claudevdm
left a comment
There was a problem hiding this comment.
I think this looks fine, but not sure it adresses @damccorm comment from previous #39206 (comment)
| * (YAML) Added WriteToDatadog transform ([#38362](https://github.com/apache/beam/issues/38362)). | ||
| * (Java) Flink 2.1 and 2.2 support is added ([#38947](https://github.com/apache/beam/issues/38947)) ([#38978](https://github.com/apache/beam/issues/38978)); Flink 1.17 and 1.18 support is dropped. | ||
| * (Python) MqttIO is now supported in Python via cross-language ([#21060](https://github.com/apache/beam/issues/21060)). | ||
| * (Python) Reduced runtime type-check overhead by combining input and output checks into a single wrapper ([#20226](https://github.com/apache/beam/issues/20226)). |
There was a problem hiding this comment.
2.75 has been released already. Please move this to future release
|
R: @damccorm do you have any concerns with this? I saw a previous attempt you asked for changes #39206 (comment) |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Reviewing now, I think this one is ok - it is not making other non-functional changes, there are just some changes to make sure we correctly preserve additional variables we're passing into the function Guessing AI caught this distinction both times? But I think it is fine to take it |
6190609 to
72d2196
Compare
Summary
Fixes #20226
This PR reduces runtime type-checking overhead in the Python SDK by consolidating two nested
DoFnwrappers into a singleRuntimeTypeCheckWrapperDoFn.Previously, input and output checks were handled by separate wrappers. The merged wrapper aims to preserve existing type-checking behavior while eliminating one level of Python call indirection per processed element.
Changes
OutputCheckWrapperDoFnandTypeCheckWrapperDoFnintoRuntimeTypeCheckWrapperDoFn.processfunction while avoiding storing a bound method on the wrapper, preserving statefulDoFnvalidation.DoFncompatibility.Tests
apache_beam/typehints/typecheck_test.py: 18 passed.apache_beam/transforms/ptransform_test.py: 202 passed, 4 skipped.Performance
Compared the original and modified implementations using Beam's existing
runtime_type_check_microbenchmark.Configuration: 20
DoFns and three timing repetitions per configuration.At 2,001 elements, the measured runtime was approximately 4.5% lower for simple type hints and 5.2% lower for nested type hints in this local run.
These measurements are preliminary and may vary by environment. They demonstrate the results of this benchmark configuration, not a guaranteed improvement for all pipelines.