-
Notifications
You must be signed in to change notification settings - Fork 59
perf(ebean-dao): collapse N per-aspect SQL queries into 1 in batchGetUnion #622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1319,6 +1319,16 @@ private List<EbeanMetadataAspect> batchGet(@Nonnull Set<AspectKey<URN, ? extends | |
| final List<EbeanMetadataAspect> oneStatementResult = batchGetHelper(new ArrayList<>(keys), keysCount, position); | ||
| finalResult.addAll(oneStatementResult); | ||
| } | ||
|
|
||
| // For DUAL_SCHEMA: compare the accumulated old-schema results (paged) against a single | ||
| // unpaged new-schema fetch. Doing the comparison here (rather than per-page in batchGetHelper) | ||
| // ensures both sides cover the same key set when keys.size() > keysCount. | ||
| boolean nonLatestVersionFlag = keys.stream().anyMatch(key -> key.getVersion() != LATEST_VERSION); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm this makes the code super hard to review and maintain, can we simply use different method for old and new schema, or even, is old schema still alive? can we just remove those comparing code. |
||
| if (!nonLatestVersionFlag && _schemaConfig == SchemaConfig.DUAL_SCHEMA && _localAccess != null) { | ||
| final List<EbeanMetadataAspect> resultsNewSchema = | ||
| _localAccess.batchGetUnion(new ArrayList<>(keys), keys.size(), 0, false, false); | ||
| EBeanDAOUtils.compareResults(finalResult, resultsNewSchema, "batchGet"); | ||
| } | ||
| return finalResult; | ||
| } | ||
|
|
||
|
|
@@ -1383,16 +1393,19 @@ List<EbeanMetadataAspect> batchGetHelper(@Nonnull List<AspectKey<URN, ? extends | |
| } | ||
|
|
||
| if (_schemaConfig == SchemaConfig.NEW_SCHEMA_ONLY) { | ||
| return _localAccess.batchGetUnion(keys, keysCount, position, false, false); | ||
| // For new schema, all aspects are columns in a single SELECT — no need for keysCount pagination. | ||
| // The first call (position=0) fetches everything; subsequent pages return empty to avoid duplicates. | ||
| if (position > 0) { | ||
| return Collections.emptyList(); | ||
| } | ||
| return _localAccess.batchGetUnion(keys, keys.size(), 0, false, false); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems to me is that we are forcing to not use pagination. then should we just simply remove that |
||
| } | ||
|
|
||
| if (_schemaConfig == SchemaConfig.DUAL_SCHEMA) { | ||
| // Compare results from both new and old schemas | ||
| final List<EbeanMetadataAspect> resultsOldSchema = batchGetUnion(keys, keysCount, position); | ||
| final List<EbeanMetadataAspect> resultsNewSchema = | ||
| _localAccess.batchGetUnion(keys, keysCount, position, false, false); | ||
| EBeanDAOUtils.compareResults(resultsOldSchema, resultsNewSchema, "batchGet"); | ||
| return resultsOldSchema; | ||
| // Return paginated old-schema results per page; the cross-schema comparison is hoisted to | ||
| // batchGet() so both sides cover the same key set (avoiding spurious mismatches when | ||
| // keys.size() > keysCount). | ||
| return batchGetUnion(keys, keysCount, position); | ||
| } | ||
|
|
||
| log.error("Please check that the SchemaConfig supplied to EbeanLocalDAO constructor is valid."); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -238,6 +238,57 @@ public static <ASPECT extends RecordTemplate> String createAspectReadSql(@Nonnul | |
| return stringBuilder.toString(); | ||
| } | ||
|
|
||
| /** | ||
| * Create a single SQL statement to read multiple aspect columns for a set of URNs. | ||
| * Unlike {@link #createAspectReadSql} which generates one SQL per aspect class, this generates a single query | ||
| * selecting all requested aspect columns at once. The gma_deleted check is NOT included in SQL — it must be | ||
| * handled in Java by the caller (checking each aspect column individually after retrieval). | ||
| * | ||
| * @param aspectColumnNames the set of aspect column names to select (e.g., "a_status", "a_ownership") | ||
| * @param urns the set of URNs to query | ||
| * @param includeSoftDeleted if true, omits deleted_ts IS NULL filter and includes deleted_ts in SELECT | ||
| * @param isTestMode whether to use the test table | ||
| * @return SQL string for the multi-aspect read | ||
| */ | ||
| public static String createMultiAspectReadSql(@Nonnull Set<String> aspectColumnNames, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how too we avoid sql injection here? |
||
| @Nonnull Set<Urn> urns, boolean includeSoftDeleted, boolean isTestMode) { | ||
| if (urns.isEmpty()) { | ||
| throw new IllegalArgumentException("Need at least 1 urn to query."); | ||
| } | ||
| if (aspectColumnNames.isEmpty()) { | ||
| throw new IllegalArgumentException("Need at least 1 aspect column to query."); | ||
| } | ||
|
|
||
| final Urn firstUrn = urns.iterator().next(); | ||
| final String firstEntityType = firstUrn.getEntityType(); | ||
| if (urns.stream().anyMatch(u -> !u.getEntityType().equals(firstEntityType))) { | ||
| throw new IllegalArgumentException("All URNs must belong to the same entity type"); | ||
| } | ||
| final String tableName = isTestMode ? getTestTableName(firstUrn) : getTableName(firstUrn); | ||
|
|
||
| // Build column list: urn, <aspect columns>, lastmodifiedon, lastmodifiedby, createdfor [, deleted_ts] | ||
| StringBuilder sb = new StringBuilder("SELECT urn, "); | ||
| sb.append(String.join(", ", aspectColumnNames)); | ||
| sb.append(", lastmodifiedon, lastmodifiedby, createdfor"); | ||
| if (includeSoftDeleted) { | ||
| sb.append(", deleted_ts"); | ||
| } | ||
| sb.append(" FROM ").append(tableName); | ||
|
|
||
| // WHERE urn IN (...) | ||
| String urnList = urns.stream() | ||
| .map(urn -> "'" + escapeReservedCharInUrn(urn.toString()) + "'") | ||
| .collect(Collectors.joining(", ")); | ||
| sb.append(" WHERE urn IN (").append(urnList).append(")"); | ||
|
|
||
| // Add deleted_ts filter when not including soft-deleted entities | ||
| if (!includeSoftDeleted) { | ||
| sb.append(" AND ").append(DELETED_TS_IS_NULL_CHECK); | ||
| } | ||
|
|
||
| return sb.toString(); | ||
| } | ||
|
|
||
| /** | ||
| * List all the aspect record (0 or 1) for a given entity urn and aspect type. | ||
| * @param aspectClass aspect type | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to make sure it's the same table name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually here we don't support different urn type, but seems like we do support that previously?