rishabhdaim commented on code in PR #3075:
URL: https://github.com/apache/jackrabbit-oak/pull/3075#discussion_r3764160791
##########
oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticOrderByTest.java:
##########
@@ -40,4 +47,39 @@ protected void createTestIndexNode() {
setTraversalEnabled(false);
}
+ /**
+ * OAK-12343: Elastic counterpart to the Lucene type=Date limitation.
Elastic sorts missing
+ * (non-date) values last instead of as the epoch, so ORDER BY returns the
valid dates in order
+ * with the non-date value last - it does not mis-order or drop rows the
way Lucene does
+ * (LuceneOrderByTest#orderByNonDateValuesWithDateTypeIsIncorrect).
+ *
+ * TODO(OAK-12344): if Lucene is aligned with this, this becomes the
shared expectation.
+ */
+ @Test
+ public void orderByNonDateValuesWithDateTypeSortsMissingLast() throws
Exception {
+ IndexDefinitionBuilder builder = createIndexDefinitionBuilder();
+ builder.evaluatePathRestrictions();
Review Comment:
The Elastic test does not cover the no-path-evaluation branch that the new
Lucene test explicitly claims returns rows on Elastic.
When every type=Date value is non-date and evaluatePathRestrictions() is
absent, Lucene's numeric-range plan returns no rows; this sibling test
exercises a different plan with a foo predicate and path evaluation. An Elastic
regression on the reported failure path would therefore pass.
Please add an Elastic test mirroring
orderByNonDateStringValuesWithDateTypeReturnsNoResults: omit
evaluatePathRestrictions(), index only ordered dt, insert three non-date
values, execute the same descendant query with option(traversal fail), and
assert all three paths are returned. Compare as a set or sort the paths because
all values are missing and their relative order is unspecified.
##########
oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneDocumentMaker.java:
##########
@@ -73,6 +73,10 @@ public class LuceneDocumentMaker extends
FulltextDocumentMaker<Document> {
private static final String LOG_KEY_IGNORING_FACET_PROPERTY = "Ignoring
facet property";
private static final String LOG_KEY_UNKNOWN = "Unknown";
+ private static final String ORDERED_CONVERT_WARN =
+ "[{}] Ignoring ordered value for property {} (type {}): not
convertible to the declared "
+ + "type {} at {}. ORDER BY may return incorrect or no results -
leave type unset or use type=String.";
Review Comment:
The warning recommends type=String for every ordered conversion failure,
although that can corrupt numeric ordering.
This catch handles LONG, DOUBLE, BOOLEAN, and DATE. For a malformed value in
a numerically ordered property, following this advice changes values such as 2
and 10 to lexicographic ordering, directing operators toward another incorrect
result.
Please keep the common warning diagnostic generic and append the type=String
workaround only for DATE, or provide type-specific remediation. Add a JUnit 4
log-capture test for a type=Long ordered property with 2, 10, and invalid
values to verify the warning does not recommend type=String.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
