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.
##########
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();
Review Comment:
Move the duplicated cross-backend fixture into OrderByCommonTest.
These lines duplicate the Lucene fixture, while three pass-through methods
were added to OrderByCommonTest solely to support both copies. If either
fixture changes during OAK-12344, the tests may stop comparing identical inputs
and conceal backend drift.
Please add a protected shared helper in OrderByCommonTest containing the
common index, content, commit, query, and assertion setup using indexOptions
directly. Have each backend test call it with its backend-specific expected
order, then remove the three pass-through methods and unused imports.
##########
oak-doc/src/site/markdown/query/lucene.md:
##########
@@ -503,6 +503,13 @@ type
Mostly inferred from the indexed value. However in some cases where same
property
type is not used consistently across various nodes then it would recommended
to specify the type explicitly.
+
+ Note: with `type=Date`, values that are not valid ISO-8601 dates are not
indexed as dates, so
Review Comment:
The note conflates sorting failures with result filtering, making the
failure mode and workaround difficult to understand.
The phrase 'if the query relies on that property to select nodes' suggests
that ORDER BY itself filters rows, while the actual distinction is between
sorting and using the property in a constraint. Readers may also miss which
workaround applies to which behavior.
Please replace the note with: 'Note: with `type=Date`, values that are not
valid ISO-8601 dates are omitted from the date sort field. As a result, `order
by` can produce an incorrect order; a query that uses the property in a
constraint can also return no matching nodes. For inconsistent values, omit
`type` or use `type=String`. String values sort lexicographically and match
chronological order only when timestamps use a uniform format, precision, and
zone (for example, UTC `Z`).'
--
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]
