Repository navigation
Conversation
mapJoinedProps filtered every target property for every row, then built and checked an aliased key for each one, so hydrating a partially loaded joined relation cost time proportional to the target entity's column count instead of the selected columns. The columns are the same on every row, so resolve the target props that have one, and the non-persistent ones, on the first row of each join path and reuse them. Refs mikro-orm#8460
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #8460. This is the first of the two paths in the issue: the per-row work in
AbstractSqlDriver.mapJoinedProps. The second,EntityFactory.mergeDatare-diffing an identity-map entity on every row, touches the merge rules for user-changed values. I'll send it as a separate PR, so this one stays reviewable on its own.For every row of a joined result,
mapJoinedPropsfiltered all of the target's properties (shouldHaveColumn), filtered them again for the non-persistent ones, and calledmapJoinedPropfor each. That call builds`${relationAlias}__${name}`and finds no such column for anything outsidefields, and the delete loop then visits the same props. Withfields: ['to.pid']on a wide target, nearly all of that is spent on columns that were never selected.Every row of one result has the same columns. So on the first row of each join path, this resolves the target props that have a column in the row (
typeof root[alias] !== 'undefined', the same testmapJoinedPropuses to skip a prop), plus the non-persistent ones. Both lists are kept in the per-querymapthatQueryBuilderalready passes tomapResult, keyed by the join path, and reused for the remaining rows. A row whose relation isnullstill has the columns (withnullvalues), so it gives the same list. Callers that don't pass amapget a fresh one per row, as before, so nothing is shared beyond one result.Numbers
These come from the issue's reproduction (SQLite in memory, 5,000 edges to 500 targets,
find(Edge, {}, { populate: ['to'], fields: ['to.pid'], strategy: 'joined' })) on macOS arm64 with Node 26. Each figure is the median of 15 runs after 3 warm-ups. I ran master and this branch alternately, twice each:The remaining growth with column count is the
mergeDatapath.Tests
tests/issues/GH8460.test.ts(SQLite), three tests:towithfields: ['to.pid']over 5 rows. The first row's relation isnull, so the list is resolved from an all-null row.pidis right on every row, and the unselected columns stay unloaded.fields(to.pidandfrom.col1, plusfrom.owner.name). Each path gets its own columns, and the populate takes one query (mockLogger).to.ownerkeeps every column.They pass on
mastertoo, since this doesn't change behaviour. They guard the cache. Each mutation below was run againstGH8460.test.tsand the SQLitepartial-loading/GH*tests (14 tests, all passing unmutated):One mutation fails nothing: dropping the non-persistent-props loop. It fails no test here, nor in
EntityManager.pglite.test.ts,GH7023or the polymorphic SQLite tests, so that branch (from the:refrefactor) has no test onmastereither. The list itself is unchanged, only computed once.What I ran:
yarn vitest run) on this branch and onmaster733c05d3, against the repo's docker-compose Postgres, MySQL 9 and MariaDB, with JSON reports. The set of failing tests is identical on both: nothing fails only on this branch, and nothing only on master. About half of the failing files are the MongoDB, MSSQL and Oracle suites, which I don't have running. I haven't set up the/etc/hostsentries either, and I didn't go through the remaining shared failures one by one.yarn buildinpackages/sql, andyarn lint(oxlint with type-aware rules and tsgo): clean.