You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "Ma77Ball (via GitHub)" <gi...@apache.org> on 2026/06/10 02:11:25 UTC
[PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Ma77Ball opened a new pull request, #5599:
URL: https://github.com/apache/texera/pull/5599
### What changes were proposed in this PR?
- Replace the per-read `deepcopy` in `Tuple.as_dict()` (`amber/src/main/python/core/models/tuple.py`) with a shallow copy, so reading a tuple no longer recursively clones every field value; cost now scales with field count instead of total field byte size.
- This path is hot: `as_dict()` backs `as_series()` (per-tuple in the batch operator path) and `as_key_value_pairs()`; a tuple carrying a large binary field previously duplicated that whole payload on every read.
- The deepcopy's isolation was unnecessary: `as_dict()` has no callers outside `Tuple`, its two users immediately build a new container, and the Tuple's mutators only reassign dict slots (never mutate a value in place), so a shallow copy preserves the independent-dict contract.
- Remove the now-unused `from copy import deepcopy` import and document why the shallow copy is safe.
### Any related issues, documentation, discussions?
N/A
### How was this PR tested?
- Existing tests only, no behavior change. Run `cd amber/src/main/python && python -m pytest ../../test/python/core/models/test_tuple.py -q`, expect 23 passed (covers `as_dict`/`as_series`/`as_key_value_pairs`).
- Run `cd amber/src/main/python && python -m pytest ../../test/python/core/runnables/test_main_loop.py ../../test/python/core/architecture/managers/test_tuple_processing_manager.py -q`, expect 22 passed (exercises the batch read path that calls `as_series`).
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang merged PR #5599:
URL: https://github.com/apache/texera/pull/5599
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "Ma77Ball (via GitHub)" <gi...@apache.org>.
Ma77Ball commented on PR #5599:
URL: https://github.com/apache/texera/pull/5599#issuecomment-4676949325
Sounds good, I will wait on this pr and please lmk when the other pr is done and ready to test the performance of this feature.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "Ma77Ball (via GitHub)" <gi...@apache.org>.
Ma77Ball commented on PR #5599:
URL: https://github.com/apache/texera/pull/5599#issuecomment-4669314154
@yicong thebissue is the one just before #5598
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "Ma77Ball (via GitHub)" <gi...@apache.org>.
Ma77Ball commented on code in PR #5599:
URL: https://github.com/apache/texera/pull/5599#discussion_r3405441417
##########
amber/src/main/python/core/models/tuple.py:
##########
@@ -230,21 +230,43 @@ def __setitem__(self, field_name: str, field_value: Field) -> None:
def as_series(self) -> pandas.Series:
"""Convert the tuple to Pandas series format"""
- return pandas.Series(self.as_dict())
+ return pandas.Series(self.copy())
+
+ def _evaluate_all_fields(self) -> None:
+ # resolve any lazy field accessors in place
+ for i in self.get_field_names():
+ self.__getitem__(i)
+
+ def copy(self) -> "OrderedDict[str, Field]":
+ """
+ Return a shallow copy of this tuple's field data.
+ Fields will be fetched from accessor if absent.
+
+ The dict is independent (keys may be added/removed/reassigned) but field
+ values are shared by reference. Safe because the tuple only reassigns
+ field slots, never mutates a value in place; avoids per-read deepcopy
+ cost for tuples carrying large field values. Use ``as_dict`` when value
+ isolation is required.
+
+ :return: dict with all the fields
+ """
+ self._evaluate_all_fields()
+ return self._field_data.copy()
def as_dict(self) -> "OrderedDict[str, Field]":
Review Comment:
done
##########
amber/src/main/python/core/models/tuple.py:
##########
@@ -230,21 +230,43 @@ def __setitem__(self, field_name: str, field_value: Field) -> None:
def as_series(self) -> pandas.Series:
"""Convert the tuple to Pandas series format"""
- return pandas.Series(self.as_dict())
+ return pandas.Series(self.copy())
+
+ def _evaluate_all_fields(self) -> None:
+ # resolve any lazy field accessors in place
+ for i in self.get_field_names():
+ self.__getitem__(i)
+
+ def copy(self) -> "OrderedDict[str, Field]":
Review Comment:
done
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #5599:
URL: https://github.com/apache/texera/pull/5599#issuecomment-4666123362
hmm, could not find an issue or discussion with this change. I will close it for now. Please fix and reopen it, thanks!
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang closed pull request #5599: perf(pyamber): avoid per-read deepcopy in Tuple.as_dict()
URL: https://github.com/apache/texera/pull/5599
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on code in PR #5599:
URL: https://github.com/apache/texera/pull/5599#discussion_r3401836528
##########
amber/src/main/python/core/models/tuple.py:
##########
@@ -230,21 +230,43 @@ def __setitem__(self, field_name: str, field_value: Field) -> None:
def as_series(self) -> pandas.Series:
"""Convert the tuple to Pandas series format"""
- return pandas.Series(self.as_dict())
+ return pandas.Series(self.copy())
+
+ def _evaluate_all_fields(self) -> None:
+ # resolve any lazy field accessors in place
+ for i in self.get_field_names():
+ self.__getitem__(i)
+
+ def copy(self) -> "OrderedDict[str, Field]":
+ """
+ Return a shallow copy of this tuple's field data.
+ Fields will be fetched from accessor if absent.
+
+ The dict is independent (keys may be added/removed/reassigned) but field
+ values are shared by reference. Safe because the tuple only reassigns
+ field slots, never mutates a value in place; avoids per-read deepcopy
+ cost for tuples carrying large field values. Use ``as_dict`` when value
+ isolation is required.
+
+ :return: dict with all the fields
+ """
+ self._evaluate_all_fields()
+ return self._field_data.copy()
def as_dict(self) -> "OrderedDict[str, Field]":
Review Comment:
Let's combine them as `as_dict(deepcopy=False)`
The callsites still need to use as_dict.
##########
amber/src/main/python/core/models/tuple.py:
##########
@@ -230,21 +230,43 @@ def __setitem__(self, field_name: str, field_value: Field) -> None:
def as_series(self) -> pandas.Series:
"""Convert the tuple to Pandas series format"""
- return pandas.Series(self.as_dict())
+ return pandas.Series(self.copy())
+
+ def _evaluate_all_fields(self) -> None:
+ # resolve any lazy field accessors in place
+ for i in self.get_field_names():
+ self.__getitem__(i)
+
+ def copy(self) -> "OrderedDict[str, Field]":
Review Comment:
Copy of a tuple should not return a dict. Let's remove this api.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5599:
URL: https://github.com/apache/texera/pull/5599#issuecomment-4685853650
<!-- texera-benchmarks-comment -->
## Arrow Flight E2E bench
| batch | schema_w | str_len | n_batches | tuples/s | MB/s | p50 ms | p99 ms | total ms |
|---:|---:|---:|---:|---:|---:|---:|---:|---:|
| 10 | 10 | 64 | 20 | 439 | 0.268 | 21.64 | 33.43 | 455.48 |
| 100 | 10 | 64 | 20 | 941 | 0.574 | 103.93 | 139.28 | 2125.28 |
| 1000 | 10 | 64 | 20 | 1132 | 0.691 | 883.21 | 921.83 | 17672.76 |
<details><summary>Raw CSV</summary>
```csv
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,455.48,200,128000,439,0.268,21637.91,33425.92,33425.92
1,100,10,64,20,2125.28,2000,1280000,941,0.574,103932.00,139281.23,139281.23
2,1000,10,64,20,17672.76,20000,12800000,1132,0.691,883214.63,921834.26,921834.26
```
</details>
[Full workflow run](https://github.com/apache/texera/actions/runs/27382994286)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5599:
URL: https://github.com/apache/texera/pull/5599#issuecomment-4665840133
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/5599?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 51.57%. Comparing base ([`7a6c22a`](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`910012f`](https://app.codecov.io/gh/apache/texera/commit/910012fafe4166f9ae227a0cc3554254bda03c7a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 41 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #5599 +/- ##
============================================
+ Coverage 51.51% 51.57% +0.06%
Complexity 2420 2420
============================================
Files 1054 1054
Lines 40926 40985 +59
Branches 4382 4382
============================================
+ Hits 21081 21137 +56
- Misses 18622 18625 +3
Partials 1223 1223
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.89% <ø> (ø)` | | Carriedforward from [7a6c22a](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.76% <ø> (ø)` | | Carriedforward from [7a6c22a](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.01% <ø> (ø)` | | Carriedforward from [7a6c22a](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `1.38% <ø> (ø)` | | Carriedforward from [7a6c22a](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `54.68% <ø> (ø)` | | Carriedforward from [7a6c22a](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.42% <ø> (ø)` | | Carriedforward from [7a6c22a](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.26% <ø> (ø)` | | Carriedforward from [7a6c22a](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.71% <100.00%> (+0.02%)` | :arrow_up: | |
| [python](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.84% <ø> (-0.01%)` | :arrow_down: | Carriedforward from [7a6c22a](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/5599/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.39% <ø> (ø)` | | Carriedforward from [7a6c22a](https://app.codecov.io/gh/apache/texera/commit/7a6c22a09b0efd60cee35e106dbbc66de2a73a55?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/5599?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #5599:
URL: https://github.com/apache/texera/pull/5599#issuecomment-4675756461
Also, if not urgent, I want to put this PR on hold after #5557 , so that we can see the performance improvement.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(pyamber): avoid per-read deepcopy in Tuple.as_dict() [texera]
Posted by "Yicong-Huang (via GitHub)" <gi...@apache.org>.
Yicong-Huang commented on PR #5599:
URL: https://github.com/apache/texera/pull/5599#issuecomment-4685833113
I updated the branch to see if it can trigger benchmark result.
Also @Ma77Ball please update PR description whenever you made changes to PR content.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org