Fix inconsistent search results for date-related queries fixes#12296 #12724
Conversation
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
| @@ -261,6 +269,75 @@ private SqlQueryNode getFieldQueryNode(String field, String term, EnumSet<Search | |||
| } | |||
| } | |||
|
|
|||
| private SqlQueryNode buildTwoFieldsUnionQuery(String field1, String field2, String operator, String prefixSuffix, String term) { | |||
There was a problem hiding this comment.
Method lacks JavaDoc documentation despite being a complex method handling SQL query construction with multiple parameters and specific logic for date/year fields.
| return new SqlQueryNode("cte" + cteCounter++); | ||
| } | ||
|
|
||
| private SqlQueryNode buildTwoFieldsNegationQuery(String field1, String field2, String operator, String prefixSuffix, String term) { |
There was a problem hiding this comment.
Complex method handling SQL negation queries lacks JavaDoc documentation. The method's purpose and parameter usage should be clearly documented for maintainability.
|
Jabref treats date and year as alias fields. And some others as well. |
|
| @@ -228,6 +228,14 @@ private SqlQueryNode getFieldQueryNode(String field, String term, EnumSet<Search | |||
| term = escapeTermForSql(term); | |||
| } | |||
|
|
|||
| if (field.equalsIgnoreCase(StandardField.DATE.getName()) || field.equalsIgnoreCase(StandardField.YEAR.getName())) { | |||
There was a problem hiding this comment.
I think, this is the wrong approach. org.jabref.model.entry.EntryConverter#FIELD_ALIASES needs to be used - maybe all fields "unzipped" into postgres
Maybe, also a "view" in postgres needs to be generated - a derived_date covering the date and then the query on the month/day/... go to this view.. The view could offer all fields values pre-calculated.
There was a problem hiding this comment.
My initial thought was that the issue was only related to the date and year fields, so my first approach focused on that. After your clarification, I realized the need to handle all date-related fields properly.
I’ve now updated the solution accordingly.
There was a problem hiding this comment.
@koppor do you think there’s still a need to create a view for this?
There was a problem hiding this comment.
A view can be a next step. A view typically helps to gain performance and reduce memory consumption.
AbdoMostfa2
force-pushed
the
fix-for-issue-12296
branch
from
1f8ead4 to
e4f9283
Compare
AbdoMostfa2
changed the title
| @@ -47,6 +47,7 @@ public class BibFieldsIndexer { | |||
| private final String splitValuesTable; | |||
| private final String schemaSplitValuesTableReference; | |||
| private final Character keywordSeparator; | |||
| private final Field[] dateFields = new Field[] {StandardField.DATE, StandardField.YEAR, StandardField.MONTH, StandardField.DAY}; | |||
There was a problem hiding this comment.
Use modern Java data structures: Set<Field> seems to pbe approivate?
| addBatch(preparedStatement, entryId, field, value, resolvedFieldLatexFree.orElse("")); | ||
|
|
||
| // Skip indexing of date-related fields separately to ensure proper handling later in the process. | ||
| if (field != StandardField.DATE && field != StandardField.YEAR && field != StandardField.MONTH && field != StandardField.DAY) { |
There was a problem hiding this comment.
Why not using dateFields.contains(field)?
| String dateValue = resolvedDateValue.orElse(""); | ||
| addBatch(preparedStatement, entryId, dateField, dateValue); |
There was a problem hiding this comment.
I know, Optionals are hard to learn, please:
Use the methods of java.util.Optional. ifPresent.
NOT
Optional<String> resolved = bibEntry.getResolvedFieldOrAlias(...);
String value = resolved.orElse("");
doSomething(value)BUT
Optional<String> resolved = bibEntry.getResolvedFieldOrAlias(...);
resolved.ifPresent(value -> doSomething(value));| FIELD_VALUE_TRANSFORMED, FIELD_VALUE_TRANSFORMED); | ||
| String entryId = entry.getId(); | ||
| // If the updated field is date-related, re-index all date fields to overwrite the previous value. | ||
| if (field == StandardField.DATE || field == StandardField.YEAR || field == StandardField.MONTH || field == StandardField.DAY) { |
| String dateValue = resolvedDateValue.orElse(""); | ||
| addBatch(preparedStatement, entryId, dateField, dateValue); |
| String insertDateFieldQuery = """ | ||
| INSERT INTO %s ("%s", "%s", "%s", "%s") | ||
| VALUES (?, ?, ?, ?) | ||
| ON CONFLICT ("%s", "%s") |
There was a problem hiding this comment.
ON CONFLICT is not clear to me. Add Java coment
There was a problem hiding this comment.
This handles situations where the user inserts conflicting data, such as date=2025-03-17 and year=2026. In this case, it will overwrite the previously calculated year. Should I ignore this conflict instead?
| } | ||
|
|
| @@ -325,7 +359,6 @@ private void insertField(BibEntry entry, Field field) { | |||
| FIELD_VALUE_TRANSFORMED); | |||
|
|
|||
| try (PreparedStatement preparedStatement = connection.prepareStatement(insertIntoSplitTable)) { | |||
| String entryId = entry.getId(); | |||
There was a problem hiding this comment.
I don't know why this part of the code is touched. I totally forgot about the use of the split fields. I asked @LoayGhreeb for support at documentaiotn
There was a problem hiding this comment.
I think the Split Table handles multi-valued fields like keywords or groups. I did not remove it, but the entryId is calculated above.
There was a problem hiding this comment.
Could you add a Java comment to org.jabref.model.search.PostgreConstants#getSplitTableSchemaReference please?
…date, Date, year)
AbdoMostfa2
force-pushed
the
fix-for-issue-12296
branch
from
e62842d to
034b28f
Compare
src/main/java/org/jabref/logic/search/indexing/BibFieldsIndexer.java
Outdated
Show resolved
Hide resolved
…r.java Co-authored-by: Christoph <siedlerkiller@gmail.com>
…jabref into fix-for-issue-12296
| if (!DATE_FIELDS.contains(field)) { | ||
| Optional<String> resolvedFieldLatexFree = bibEntry.getResolvedFieldOrAliasLatexFree(field, this.databaseContext.getDatabase()); | ||
| assert resolvedFieldLatexFree.isPresent(); | ||
| addBatch(preparedStatement, entryId, field, value, resolvedFieldLatexFree.orElse("")); | ||
| } |
There was a problem hiding this comment.
The code uses assert and orElse("") for Optional handling instead of proper Optional methods. Should use ifPresent() for better Optional handling.
There was a problem hiding this comment.
You can use resolvedFieldLatexFree.ifPresent(content -> ...)
There was a problem hiding this comment.
I understand the point about using ifPresent() for better Optional handling. However, I deliberately retained the assertion because the comment emphasizes detecting potential flaws.
There was a problem hiding this comment.
Do both.
Consistency with other code.
We cannot change current tooling easily.
There was a problem hiding this comment.
You can also use get() and ignore the bot comment. Seems to be best (seeing these two lines only)
Fix inconsistent search results for date-related queries
This update ensures that searching for different date fields (e.g., year=2012, date=2012, Date=2012) returns consistent results. Previously, these queries produced different results due to variations in field names.
Closes #12296
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)