You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2022/08/09 01:47:01 UTC
[GitHub] [james-project] chibenwa commented on a diff in pull request #1111: JAMES-3799 Optimize memory requirements of SimpleMessageSearchIndex
chibenwa commented on code in PR #1111:
URL: https://github.com/apache/james-project/pull/1111#discussion_r940804516
##########
mailbox/store/src/main/java/org/apache/james/mailbox/store/search/SimpleMessageSearchIndex.java:
##########
@@ -113,6 +114,67 @@ private static UidCriterion findConjugatedUidCriterion(List<Criterion> crits) {
return null;
}
+ /**
+ * Walks down the query tree's conjunctions to find the highest necessary mail fetch type.
+ * @param crits - list of Criterion to search from
+ * @return required fetch type - metadata, headers, or full
+ */
+ private static FetchType getFetchTypeForCriteria(List<Criterion> crits) {
+ FetchType maximum = FetchType.METADATA;
+ for (Criterion crit : crits) {
+ if (crit instanceof ConjunctionCriterion) {
+ return getFetchTypeForCriteria(((ConjunctionCriterion) crit)
+ .getCriteria());
+ } else {
+ maximum = maxFetchType(maximum, getFetchTypeForCriterion(crit));
+ }
+ }
+ return maximum;
+ }
+
+ private static FetchType getFetchTypeForCriterion(Criterion crit) {
+ if (crit instanceof SearchQuery.AllCriterion || crit instanceof SearchQuery.TextCriterion) {
+ return FetchType.FULL;
+ } else
+ if (crit instanceof SearchQuery.HeaderCriterion || crit instanceof SearchQuery.MimeMessageIDCriterion) {
+ return FetchType.HEADERS;
+ } else {
+ return FetchType.METADATA;
+ }
+ }
Review Comment:
If there is a conjunction criteria, we still need to take into account the next criterias.
I think here is a very good example where functional programing might be more efficient? (map - reduce)
For instance consider `AND(CritA, CritB), CritC` then with your current code CritC is ignored. Which is a bug.
```suggestion
private static FetchType getFetchTypeForCriteria(List<Criterion> crits) {
return crits.stream()
.map(crit -> getFetchTypeForCriterion(crit))
.reduce((a, b) -> maxFetchType(a, b))
.orElse(FetchType.METADATA);
}
private static FetchType getFetchTypeForCriterion(Criterion crit) {
if (crit instanceof ConjunctionCriterion) {
return getFetchTypeForCriteria(((ConjunctionCriterion) crit)
.getCriteria())
}
if (crit instanceof SearchQuery.AllCriterion || crit instanceof SearchQuery.TextCriterion) {
return FetchType.FULL;
}
if (crit instanceof SearchQuery.HeaderCriterion || crit instanceof SearchQuery.MimeMessageIDCriterion) {
return FetchType.HEADERS;
}
return FetchType.METADATA;
}
```
##########
mailbox/store/src/main/java/org/apache/james/mailbox/store/search/SimpleMessageSearchIndex.java:
##########
@@ -113,6 +114,67 @@ private static UidCriterion findConjugatedUidCriterion(List<Criterion> crits) {
return null;
}
+ /**
+ * Walks down the query tree's conjunctions to find the highest necessary mail fetch type.
+ * @param crits - list of Criterion to search from
+ * @return required fetch type - metadata, headers, or full
+ */
+ private static FetchType getFetchTypeForCriteria(List<Criterion> crits) {
+ FetchType maximum = FetchType.METADATA;
+ for (Criterion crit : crits) {
+ if (crit instanceof ConjunctionCriterion) {
+ return getFetchTypeForCriteria(((ConjunctionCriterion) crit)
+ .getCriteria());
+ } else {
+ maximum = maxFetchType(maximum, getFetchTypeForCriterion(crit));
+ }
+ }
+ return maximum;
+ }
+
+ private static FetchType getFetchTypeForCriterion(Criterion crit) {
+ if (crit instanceof SearchQuery.AllCriterion || crit instanceof SearchQuery.TextCriterion) {
+ return FetchType.FULL;
+ } else
+ if (crit instanceof SearchQuery.HeaderCriterion || crit instanceof SearchQuery.MimeMessageIDCriterion) {
+ return FetchType.HEADERS;
+ } else {
+ return FetchType.METADATA;
+ }
+ }
+
+ /**
+ * Searches a list of query sort options for the highest necessary mail fetch type.
+ * @param sorts - list of Sort to search
+ * @return required fetch type - metadata or headers
+ */
+ private static FetchType getFetchTypeForSorts(List<SearchQuery.Sort> sorts) {
+ return sorts.stream()
+ .map(SimpleMessageSearchIndex::getFetchTypeForSort)
+ .reduce(FetchType.METADATA, SimpleMessageSearchIndex::maxFetchType);
+ }
+
+ private static FetchType getFetchTypeForSort(SearchQuery.Sort sort) {
+ switch (sort.getSortClause()) {
+ case Arrival:
+ case Size:
+ case Uid:
+ case Id:
+ return FetchType.METADATA;
+ case MailboxCc:
+ case MailboxFrom:
+ case MailboxTo:
+ case BaseSubject:
+ case SentDate:
+ default:
Review Comment:
Shouldn't we throw explicitly on unspecified sorts to avoid later bugs to sneak in ? (fail early?)
##########
mailbox/store/src/main/java/org/apache/james/mailbox/store/search/SimpleMessageSearchIndex.java:
##########
@@ -113,6 +114,67 @@ private static UidCriterion findConjugatedUidCriterion(List<Criterion> crits) {
return null;
}
+ /**
+ * Walks down the query tree's conjunctions to find the highest necessary mail fetch type.
+ * @param crits - list of Criterion to search from
+ * @return required fetch type - metadata, headers, or full
+ */
+ private static FetchType getFetchTypeForCriteria(List<Criterion> crits) {
+ FetchType maximum = FetchType.METADATA;
+ for (Criterion crit : crits) {
+ if (crit instanceof ConjunctionCriterion) {
+ return getFetchTypeForCriteria(((ConjunctionCriterion) crit)
+ .getCriteria());
+ } else {
+ maximum = maxFetchType(maximum, getFetchTypeForCriterion(crit));
+ }
+ }
+ return maximum;
+ }
+
+ private static FetchType getFetchTypeForCriterion(Criterion crit) {
+ if (crit instanceof SearchQuery.AllCriterion || crit instanceof SearchQuery.TextCriterion) {
+ return FetchType.FULL;
+ } else
+ if (crit instanceof SearchQuery.HeaderCriterion || crit instanceof SearchQuery.MimeMessageIDCriterion) {
+ return FetchType.HEADERS;
+ } else {
+ return FetchType.METADATA;
+ }
+ }
+
+ /**
+ * Searches a list of query sort options for the highest necessary mail fetch type.
+ * @param sorts - list of Sort to search
+ * @return required fetch type - metadata or headers
+ */
+ private static FetchType getFetchTypeForSorts(List<SearchQuery.Sort> sorts) {
+ return sorts.stream()
+ .map(SimpleMessageSearchIndex::getFetchTypeForSort)
+ .reduce(FetchType.METADATA, SimpleMessageSearchIndex::maxFetchType);
+ }
+
+ private static FetchType getFetchTypeForSort(SearchQuery.Sort sort) {
+ switch (sort.getSortClause()) {
+ case Arrival:
+ case Size:
+ case Uid:
+ case Id:
+ return FetchType.METADATA;
+ case MailboxCc:
+ case MailboxFrom:
+ case MailboxTo:
+ case BaseSubject:
+ case SentDate:
+ default:
+ return FetchType.HEADERS;
+ }
+ }
+
+ private static FetchType maxFetchType(FetchType a, FetchType b) {
+ return ArrayUtils.indexOf(FetchType.values(), a) >= ArrayUtils.indexOf(FetchType.values(), b) ? a : b;
+ }
Review Comment:
While I am ok with this hacky implementation there might be concern over the overall stability of this across java versions. I would write test for it!
Also consider using `MessageMapper.FetchType.BODY.ordinal()` instead of doing manual array index lookup.
--
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@james.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org