Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4321 +/- ##
==========================================
+ Coverage 98.65% 98.66% +0.01%
==========================================
Files 59 59
Lines 11964 12090 +126
Branches 2271 2296 +25
==========================================
+ Hits 11803 11929 +126
Misses 84 84
Partials 77 77 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
|
|
||
| def _get_label(value: Any, start_index: Any, index: int) -> str: | ||
| m: dict[str | None, Callable[[int], str]] = { |
There was a problem hiding this comment.
When we are refactoring this anyway, we could probably make this a constant outside of the function.
There was a problem hiding this comment.
Done, moved it to _NUMBERING_STYLES.
| prefix=prefix, | ||
| exception=exception, | ||
| ) | ||
| return str(index + 1) # Fallback |
There was a problem hiding this comment.
This can probably moved one level up:
| return str(index + 1) # Fallback | |
| return str(index + 1) # Fallback |
| def _get_page_range(limits: list[Any], page_count: int) -> tuple[int, int] | None: | ||
| """Get the first and last page index between the two ``/Limits`` values, if any.""" | ||
| low, high = limits[0], limits[1] | ||
| if any(isinstance(limit, float) and math.isnan(limit) for limit in (low, high)): |
There was a problem hiding this comment.
In which cases will limit be NaN?
There was a problem hiding this comment.
It can't come from a file, only from objects built by hand, so I removed the check.
| low, high = limits[0], limits[1] | ||
| if any(isinstance(limit, float) and math.isnan(limit) for limit in (low, high)): | ||
| return None | ||
| if low > page_count - 1 or high < 0: |
There was a problem hiding this comment.
We can eliminate the subtraction?
| if low > page_count - 1 or high < 0: | |
| if low >= page_count or high < 0: |
Alternatively, as we use page_count - 1 later on, consider using a dedicated variable.
| position = bisect_right(self._starts, index) - 1 | ||
| if position >= 0 and index <= self._ends[position]: | ||
| return self._first_kids[position] | ||
| if self._next_kid >= len(self._kids): |
There was a problem hiding this comment.
As self._kids is a fixed parameter passed on __init__, I guess we can calculate len(self._kids) once in __init__ as well?
| indexed once and each ``/Nums`` array is read once for all of its pages. Warnings | ||
| about malformed kids are logged once per array instead of once per page. | ||
| """ | ||
| finders: dict[int, tuple[ArrayObject, _FirstKidFinder]] = {} |
There was a problem hiding this comment.
Consider making the finder values a dedicated container for readability.
There was a problem hiding this comment.
The finder keeps its own /Kids array now, so the dict just maps to the finder.
| about malformed kids are logged once per array instead of once per page. | ||
| """ | ||
| finders: dict[int, tuple[ArrayObject, _FirstKidFinder]] = {} | ||
| leaves: dict[int, tuple[ArrayObject | None, DictionaryObject, list[int]]] = {} |
There was a problem hiding this comment.
Consider making the leaf values a dedicated container for readability.
| if id(kids) not in finders: | ||
| finders[id(kids)] = (kids, _FirstKidFinder(kids, page_count)) | ||
| finder = finders[id(kids)][1] | ||
| groups: dict[int, tuple[DictionaryObject, list[int]]] = {} |
There was a problem hiding this comment.
Consider making the group values a dedicated container for readability.
| groups.setdefault(id(kid), (kid, []))[1].append(index) | ||
| for kid, kid_indexes in groups.values(): | ||
| if not is_null_or_none(kid.get("/Kids", None)): | ||
| if level + 1 == 100: # pragma: no cover |
There was a problem hiding this comment.
Did we have this limit before? If not, where is it coming from?
There was a problem hiding this comment.
Yes, index2label() raises the same NotImplementedError at level 100. Added a comment saying so.
| leaf_labels = ( | ||
| _get_labels_from_nums(leaf_nums, leaf_indexes) | ||
| if leaf_nums is not None | ||
| else [get_label_from_nums(leaf, index) for index in leaf_indexes] |
There was a problem hiding this comment.
I find it somehow confusing to have _get_labels_from_nums and get_labels_from_nums.
There was a problem hiding this comment.
Renamed to _get_labels_for_page_indexes.
#4251 only sped up one case: a flat
/Numsarray with sorted integer keys and direct values. Everything else still callsindex2label()for each page, and that walks the tree from the start every time. So/Numswith indirect values or unsorted keys, and any/Kidstree, are still pages × label ranges.Now a
/Numsarray is read once for all pages. The pairget_label_from_nums()stops at only moves forward as the page index grows, so the keys don't need to be sorted. For/Kids, each array's kids are read in order and only as far as needed (same asindex2label()), and I keep track of which pages each kid covers. Each/Numsarray is then read once.Timings for 8,000 pages (1.0 to 1.4 MB), loading the page tree included:
/Nums, indirect values/Nums, unsorted keys/Kids, one kid/Kids, many kidsLabels and exceptions are the same as calling
index2label()page by page, and the new tests check that for several/Kidslayouts and for indirect and shared/Nums. Two small differences: a warning about a broken kid now shows up once per/Kidsarray, not once per page, and a kid whose/Limitsaren't numbers gets skipped with the existing "malformed /Limits" warning.AI assistance: Claude Code (Claude Opus 5.5) helped write and measure this, and Codex (GPT-6 Astra) reviewed it. I reviewed the change and ran the test suite.