Skip to content

Conversation

robfrank
Copy link
Collaborator

@robfrank robfrank commented Jun 1, 2025

… done so far and provide feedback for Jules to continue.

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Related issues

A list of issues either fixed, containing architectural discussions, otherwise relevant
for this Pull Request.

Additional Notes

Anything else we should know when reviewing?

Checklist

  • I have run the build using mvn clean package command
  • My unit tests cover both failure and success scenarios

… done so far and provide feedback for Jules to continue.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @robfrank, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

As requested, gemini-code-assist has reviewed the current state of the pull request titled "Jules was unable to complete the task in time. Please review the work…" authored by robfrank. Based on the provided patch content and the minimal description, this pull request introduces the foundational structure for integrating Apache Lucene as a full-text search index engine within ArcadeDB. The changes involve adding a new Maven module (arcadedb-lucene), implementing core index engine logic, providing SQL functions and operators for querying Lucene indexes, and registering these components with the ArcadeDB plugin system. While the structure is laid out, several parts appear to be initial stubs or incomplete implementations, aligning with the PR title suggesting it's work in progress requiring review and feedback.

Highlights

  • New Lucene Module: Introduces a new Maven module (lucene) to house the Lucene integration code and manage its dependencies.
  • Lucene Index Engine: Adds an OLuceneIndexEngine interface and initial implementations (OLuceneFullTextIndexEngine, OLuceneCrossClassIndexEngine) to handle the low-level interactions with the Lucene library for indexing and searching.
  • SQL Functions and Operators: Registers several new SQL functions (SEARCH_INDEX, SEARCH_CLASS, SEARCH_FIELDS, SEARCH_MORE, SEARCH_CROSS) and operators to allow users to perform Lucene-based full-text searches directly from ArcadeDB SQL queries.
  • Index Factory and Lifecycle: Includes an IndexFactoryHandler (ArcadeLuceneIndexFactoryHandler) for creating Lucene index instances and a basic ArcadeLuceneLifecycleManager for plugin registration.
  • Transaction Handling Stubs: Introduces interfaces and abstract classes (OLuceneTxChanges, OLuceneTxChangesAbstract) and initial implementations (OLuceneTxChangesMultiRid, OLuceneTxChangesSingleRid) for managing Lucene index changes within ArcadeDB transactions, although the implementations appear incomplete.
  • Core Lucene Integration Classes: Adds helper classes for building Lucene documents (OLuceneDocumentBuilder), parsing queries (OLuceneQueryBuilder, OLuceneMultiFieldQueryParser), managing analyzers (OLuceneAnalyzerFactory, OLucenePerFieldAnalyzerWrapper), and defining index types (OLuceneIndexType).
  • Placeholder Implementations: Note that several methods in ArcadeLuceneFullTextIndex and LuceneIndexCursor currently throw UnsupportedOperationException, indicating these parts of the integration are still in the early stages.

Changelog

Click here to see the changelog
  • lucene/pom.xml
    • Added new Maven module for Lucene integration.
    • Included dependencies for Apache Lucene core, analysis, query parser, spatial, and other modules.
    • Added dependencies for Spatial4j and JTS Core.
    • Configured maven-jar-plugin.
  • lucene/src/main/java/com/arcadedb/lucene/ArcadeLuceneIndexFactoryHandler.java
    • Added new IndexFactoryHandler for creating ArcadeDB Lucene indexes.
    • Parses index builder parameters including analyzer class name from properties.
    • Instantiates ArcadeLuceneFullTextIndex with database, name, unique flag, analyzer class, file path, and key types.
  • lucene/src/main/java/com/arcadedb/lucene/ArcadeLuceneLifecycleManager.java
    • Added new minimal lifecycle manager class.
    • Defines LUCENE_ALGORITHM constant.
    • Includes basic constructor with logging.
  • lucene/src/main/java/com/arcadedb/lucene/analyzer/OLuceneAnalyzerFactory.java
    • Added new factory for creating Lucene Analyzer instances.
    • Supports default, per-kind (INDEX/QUERY), and field-specific analyzers with stopwords.
    • Uses reflection to build analyzer instances.
  • lucene/src/main/java/com/arcadedb/lucene/analyzer/OLucenePerFieldAnalyzerWrapper.java
    • Added new custom DelegatingAnalyzerWrapper.
    • Allows different analyzers per field.
    • Hardcodes KeywordAnalyzer for internal fields like RID, _CLASS, _CLUSTER, _JSON.
  • lucene/src/main/java/com/arcadedb/lucene/builder/OLuceneIndexType.java
    • Added new utility class for Lucene index types.
    • Provides methods to create Lucene Fields from ArcadeDB data types.
    • Includes methods for creating Lucene Query objects (exact, RID, delete).
    • Adds key hashing utility.
  • lucene/src/main/java/com/arcadedb/lucene/engine/OLuceneCrossClassIndexEngine.java
    • Added new OLuceneIndexEngine implementation for cross-class search.
    • Implements get method to query across multiple Lucene indexes using MultiReader and OLuceneMultiFieldQueryParser.
    • Most other OIndexEngine methods are unimplemented stubs.
  • lucene/src/main/java/com/arcadedb/lucene/engine/OLuceneFullTextIndexEngine.java
    • Added new OLuceneIndexEngine implementation for full-text indexing.
    • Handles building Lucene documents and queries.
    • Provides getInTx for querying with transaction changes.
    • Includes logic for manual indexes.
  • lucene/src/main/java/com/arcadedb/lucene/engine/OLuceneIndexEngine.java
    • Added new interface extending OIndexEngine and OFreezableStorageComponent.
    • Defines Lucene-specific engine methods like buildDocument, buildQuery, indexAnalyzer, queryAnalyzer, searcher, buildTxChanges.
  • lucene/src/main/java/com/arcadedb/lucene/functions/OLuceneCrossClassFunctionsFactory.java
    • Added new SQL function factory for cross-class search.
    • Registers the OLuceneCrossClassSearchFunction.
  • lucene/src/main/java/com/arcadedb/lucene/functions/OLuceneCrossClassSearchFunction.java
    • Added new SQL function SEARCH_CROSS.
    • Uses OLuceneCrossClassIndexEngine to search across multiple Lucene indexes.
    • Handles query and metadata parameters.
  • lucene/src/main/java/com/arcadedb/lucene/functions/OLuceneFunctionsFactory.java
    • Added new SQL function factory for standard Lucene search functions.
    • Registers SEARCH_INDEX, SEARCH_FIELDS, SEARCH_CLASS, and SEARCH_MORE functions.
  • lucene/src/main/java/com/arcadedb/lucene/functions/OLuceneFunctionsUtils.java
    • Added new utility class for Lucene SQL functions.
    • Includes methods for finding indexes, managing MemoryIndex, and escaping query strings.
  • lucene/src/main/java/com/arcadedb/lucene/functions/OLuceneSearchFunctionTemplate.java
    • Added new abstract base class for Lucene SQL search functions.
    • Provides common logic for index lookup, metadata parsing, and estimation.
  • lucene/src/main/java/com/arcadedb/lucene/functions/OLuceneSearchMoreLikeThisFunction.java
    • Added new SQL function search_more.
    • Implements 'More Like This' search using Lucene's MoreLikeThis query.
    • Finds documents similar to a given set of RIDs.
  • lucene/src/main/java/com/arcadedb/lucene/functions/OLuceneSearchOnClassFunction.java
    • Added new SQL function search_class.
    • Finds the Lucene index for the current record's class and performs a search.
    • Includes inline execution logic using MemoryIndex.
  • lucene/src/main/java/com/arcadedb/lucene/functions/OLuceneSearchOnFieldsFunction.java
    • Added new SQL function search_fields.
    • Finds a Lucene index on the current record's class covering specified fields and performs a search.
    • Includes inline execution logic using MemoryIndex.
  • lucene/src/main/java/com/arcadedb/lucene/functions/OLuceneSearchOnIndexFunction.java
    • Added new SQL function search_index.
    • Finds a specific Lucene index by name and performs a search.
    • Includes inline execution logic using MemoryIndex.
  • lucene/src/main/java/com/arcadedb/lucene/index/ArcadeLuceneFullTextIndex.java
    • Added new class implementing ArcadeDB's IndexInternal interface.
    • Stores basic index metadata (database, name, unique, analyzer, path, key types).
    • Most core index methods are currently unimplemented stubs throwing UnsupportedOperationException.
  • lucene/src/main/java/com/arcadedb/lucene/index/OLuceneFullTextIndex.java
    • Added new class extending OLuceneIndexNotUnique.
    • Delegates document building, query building, and analyzer retrieval to the underlying OLuceneIndexEngine.
    • Overrides isAutomatic to include the cross-class algorithm.
  • lucene/src/main/java/com/arcadedb/lucene/query/LuceneIndexCursor.java
    • Added new class implementing ArcadeDB's IndexCursor interface.
    • Currently a stub with most methods throwing UnsupportedOperationException.
  • lucene/src/main/java/com/arcadedb/lucene/query/OLuceneQueryContext.java
    • Added new context class for Lucene queries.
    • Holds IndexSearcher, Query, Sort, transaction changes, and highlight fragments.
    • Can wrap the searcher with transaction changes using MultiReader.
  • lucene/src/main/java/com/arcadedb/lucene/tx/OLuceneTxChanges.java
    • Added new interface for managing Lucene transaction changes.
    • Defines methods for put, remove, searcher, and checking deleted/updated documents.
  • lucene/src/main/java/com/arcadedb/lucene/tx/OLuceneTxChangesAbstract.java
    • Added new abstract base class for OLuceneTxChanges.
    • Holds IndexWriters for added and deleted documents within a transaction.
    • Provides a transaction-aware searcher.
  • lucene/src/main/java/com/arcadedb/lucene/tx/OLuceneTxChangesMultiRid.java
    • Added new implementation of OLuceneTxChanges for multi-RID scenarios.
    • Uses separate writers for added and deleted documents.
    • Tracks deleted RIDs per key string.
  • lucene/src/main/java/com/arcadedb/lucene/tx/OLuceneTxChangesSingleRid.java
    • Added new implementation of OLuceneTxChanges for single-RID scenarios.
    • Tracks deleted and updated RIDs using sets.
  • lucene/src/main/resources/META-INF/services/com.arcadedb.database.index.OIndexFactory
    • Registered com.arcadedb.lucene.OLuceneIndexFactory.
    • Registered com.arcadedb.lucene.OLuceneCrossClassIndexFactory.
  • lucene/src/main/resources/META-INF/services/com.arcadedb.database.sql.functions.OSQLFunctionFactory
    • Registered com.arcadedb.lucene.functions.OLuceneFunctionsFactory.
    • Registered com.arcadedb.lucene.functions.OLuceneCrossClassFunctionsFactory.
  • lucene/src/main/resources/META-INF/services/com.arcadedb.database.sql.operator.OQueryOperatorFactory
    • Registered com.arcadedb.lucene.operator.OLuceneOperatorFactory.
  • lucene/src/main/resources/META-INF/services/com.arcadedb.index.IndexFactoryHandler
    • Registered com.arcadedb.lucene.ArcadeLuceneIndexFactoryHandler.
  • lucene/src/main/resources/plugin.json
    • Added new plugin definition file for the Lucene index integration.
  • pom.xml
    • Added the new lucene module to the main project build.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


New module takes flight,
Lucene search, shining bright,
Work in progress, seen.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Comment on lines +108 to +112
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>1.7.36</version> <!-- Specify a common version -->
</dependency>

Check notice

Code scanning / Meterian

[stability] org.slf4j:slf4j-api@1.7.36 is outdated Low

org.slf4j:slf4j-api@1.7.36 is outdated
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request lays the groundwork for Lucene integration, including the POM setup, factory handlers, function definitions, and some engine components. However, the core index implementation (ArcadeLuceneFullTextIndex) is largely a stub, and there are several critical and high-priority issues to address before this can be functional. The presence of OrientDB-specific artifacts and naming conventions also needs attention.

Key areas for Jules to focus on next would be:

  1. Implementing the methods in com.arcadedb.lucene.index.ArcadeLuceneFullTextIndex to delegate to a functional Lucene index engine (likely com.arcadedb.lucene.engine.OLuceneFullTextIndexEngine or a similar refactored component).
  2. Addressing the critical copyright and SPI file issues.
  3. Enabling and writing tests.

Summary of Findings

  • Core Implementation Incomplete: The primary class com.arcadedb.lucene.index.ArcadeLuceneFullTextIndex is mostly stubs. This is critical for functionality.
  • Plugin Metadata: The plugin.json file has an incorrect copyright notice (Orient Technologies LTD instead of Arcade Data Ltd).
  • SPI File Entries: Several Service Provider Interface (SPI) files contain entries pointing to com.orientechnologies.spatial.* classes, which seem like leftovers from OrientDB and could cause issues.
  • Resource Management: Potential resource leaks related to IndexReader and IndexSearcher in OLuceneCrossClassIndexEngine need to be reviewed.
  • Testing: Test dependencies in lucene/pom.xml are commented out, and no tests are included in this PR. Testing is crucial.
  • POM Configuration: Clarity around version management for Lucene and related dependencies in lucene/pom.xml could be improved.
  • Reflection Robustness: Reflection usage in OLuceneAnalyzerFactory for instantiating analyzers could be made more robust and clearer.
  • TODO Comments: Several TODO comments in OLuceneIndexType indicate incomplete or transitional logic that needs to be addressed.
  • Constant Placement: The LUCENE_ALGORITHM constant in ArcadeLuceneLifecycleManager might be better placed in a shared constants class if used by multiple components. (Low severity, not commented directly)
  • Hardcoded Field Names: Some internal field names (e.g., _CLASS, _JSON) are hardcoded as string literals in OLucenePerFieldAnalyzerWrapper. Using constants might be preferable. (Low severity, not commented directly)

Merge Readiness

This pull request is a good starting point for the Lucene integration module, but it is not ready for merging. The critical issues regarding the incomplete core index implementation (ArcadeLuceneFullTextIndex) and the incorrect plugin copyright must be addressed. Additionally, the high-severity concerns about SPI file entries and potential resource leaks should be resolved. I recommend that Jules focuses on these areas and on adding tests before this PR can be considered for merging. As an AI, I am not authorized to approve pull requests; this feedback is for your consideration.

Comment on lines +25 to +362

@Override
public IndexEngine getEngine() {
// Return the LuceneIndexEngine instance associated with this index
throw new UnsupportedOperationException("Not yet implemented: getEngine");
}

@Override
public boolean isValid() {
throw new UnsupportedOperationException("Not yet implemented: isValid");
}

@Override
public Map<String, String> getStats() {
// Return Lucene specific stats
throw new UnsupportedOperationException("Not yet implemented: getStats");
}

@Override
public void setStats(Map<String, String> stats) {
// Not typically set from outside
throw new UnsupportedOperationException("Not yet implemented: setStats");
}

@Override
public void compact() throws IOException {
// Trigger Lucene merge/optimize if applicable
throw new UnsupportedOperationException("Not yet implemented: compact");
}

@Override
public boolean isCompacting() {
// Check if Lucene merge/optimize is running
throw new UnsupportedOperationException("Not yet implemented: isCompacting");
}

@Override
public List<Integer> getFileIds() {
// Lucene manages its own files; this might not map directly.
throw new UnsupportedOperationException("Not yet implemented: getFileIds");
}

@Override
public int getPageSize() {
// Lucene doesn't use pages in the same way as ArcadeDB's native engine.
throw new UnsupportedOperationException("Not yet implemented: getPageSize");
}

@Override
public void setPageSize(int pageSize) {
throw new UnsupportedOperationException("Not yet implemented: setPageSize");
}

@Override
public byte getNullStrategy() {
// Get from IndexDefinition
if (this.definition == null) throw new UnsupportedOperationException("Definition not set");
return this.definition.getNullStrategy().getValue();
}

@Override
public void setNullStrategy(byte nullStrategy) {
// Set in IndexDefinition (usually immutable after creation)
throw new UnsupportedOperationException("Not yet implemented: setNullStrategy");
}

@Override
public void set(TransactionContext tx, Object[] keys, RID[] rids) throws IndexException {
// Add entries to Lucene index
throw new UnsupportedOperationException("Not yet implemented: set");
}

@Override
public void remove(TransactionContext tx, Object[] keys, Identifiable rid) throws IndexException {
// Remove specific RID associated with keys
throw new UnsupportedOperationException("Not yet implemented: remove with rid");
}

@Override
public void remove(TransactionContext tx, Object[] keys) throws IndexException {
// Remove all RIDs associated with keys
throw new UnsupportedOperationException("Not yet implemented: remove");
}

@Override
public IndexCursor range(boolean ascendingOrder) {
throw new UnsupportedOperationException("Not yet implemented: range without keys");
}

@Override
public IndexCursor range(boolean ascendingOrder, Object[] beginKeys, boolean beginKeysIncluded, Object[] endKeys, boolean endKeysIncluded, int limit, int skip) {
throw new UnsupportedOperationException("Not yet implemented: range with limit and skip");
}

@Override
public int getAssociatedBucketId() {
// Lucene indexes are not directly associated with a single bucket in the same way.
return -1; // Or derive from schema/type if applicable
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This class ArcadeLuceneFullTextIndex is the one instantiated by ArcadeLuceneIndexFactoryHandler and is intended to be the main public API for Lucene full-text indexes. However, almost all of its methods are stubbed with throw new UnsupportedOperationException(...).

This means the core functionality of the Lucene index (putting data, getting data, counting, iterating, etc.) is not yet implemented. This is the most critical part that needs to be completed for the Lucene module to be usable.

Jules, the primary task here would be to implement these methods. This typically involves:

  1. Having an instance field for the actual Lucene index engine (e.g., an instance of com.arcadedb.lucene.engine.OLuceneFullTextIndexEngine or a similar refactored component).
  2. Initializing this engine in the build() or setMetadata() methods.
  3. Delegating the calls from ArcadeLuceneFullTextIndex methods (like get, set, countEntries, iterator) to the corresponding methods on the internal engine instance.

"javaClass": "com.arcadedb.lucene.ArcadeLuceneLifecycleManager",
"parameters": {},
"description": "This is the Lucene Index integration",
"copyrights": "Orient Technologies LTD"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The copyrights field is currently set to "Orient Technologies LTD". For an ArcadeDB plugin, this should be updated to "Arcade Data Ltd" or the appropriate copyright holder for ArcadeDB. This is important for legal and branding consistency.

  "copyrights": "Arcade Data Ltd"

Comment on lines +152 to +156
OLuceneFullTextIndex fullTextIndex = (OLuceneFullTextIndex) index.getInternal();

globalAnalyzer.add((OLucenePerFieldAnalyzerWrapper) fullTextIndex.queryAnalyzer());

globalReaders.add(fullTextIndex.searcher().getIndexReader());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In the loop, fullTextIndex.searcher() is called, and then getIndexReader() on that searcher. It's crucial to manage the lifecycle of these IndexSearcher and IndexReader instances.

Specifically:

  1. Does fullTextIndex.searcher() return a new searcher that needs to be released (e.g., via fullTextIndex.release(searcher))?
  2. The IndexReader obtained is added to globalReaders. The MultiReader created on line 160 takes ownership of these readers if its closeSubReaders argument is true (which it is by default for the constructor new MultiReader(IndexReader... subReaders)). However, if fullTextIndex.searcher() itself holds resources or the reader it provides is managed, this needs careful handling to prevent leaks.

Could you confirm the resource management strategy here to ensure no IndexReader or IndexSearcher instances are leaked?

#

com.arcadedb.lucene.OLuceneIndexFactory
com.orientechnologies.spatial.OLuceneSpatialIndexFactory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The entry com.orientechnologies.spatial.OLuceneSpatialIndexFactory appears to be a leftover from OrientDB. If ArcadeDB's Lucene module does not include or intend to support this specific spatial factory (or if it's provided by a different module), this line should be removed. Including it could lead to ClassNotFoundExceptions or attempts to load unintended components during service discovery.

#

com.arcadedb.lucene.functions.OLuceneFunctionsFactory
com.orientechnologies.spatial.functions.OSpatialFunctionsFactory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the OIndexFactory file, the entry com.orientechnologies.spatial.functions.OSpatialFunctionsFactory seems to be an OrientDB artifact. If this spatial function factory isn't part of this Lucene module or ArcadeDB's core, it should likely be removed to prevent potential issues.

Comment on lines +114 to +126
<!-- Test Dependencies (Optional for now, can be added in Test Migration step) -->
<!--
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
-->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The test dependencies (JUnit) are currently commented out. The PR checklist also indicates that unit test coverage is not yet confirmed. For a robust module, comprehensive tests are essential. Are there plans to uncomment these and add unit tests soon? This is important for verifying the functionality as it's developed.

Comment on lines +95 to +106
} catch (final NoSuchMethodException e) {
Class classAnalyzer = null;
try {
classAnalyzer = Class.forName(analyzerFQN);
return (Analyzer) classAnalyzer.newInstance();
} catch (Exception e1) {
logger.error("Exception is suppressed, original exception is ", e);
//noinspection ThrowInsideCatchBlockWhichIgnoresCaughtException
throw OException.wrapException(
new OIndexException("Couldn't instantiate analyzer: public constructor not found"),
e1);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The exception handling and reflection logic in this catch (final NoSuchMethodException e) block seems a bit complex and potentially problematic:

  1. It logs the original NoSuchMethodException e with a message "Exception is suppressed, original exception is".
  2. It then attempts to instantiate the analyzer again using classAnalyzer.newInstance(), which is deprecated and can have issues with constructors throwing checked exceptions.
  3. If this second attempt fails, it catches Exception e1 and throws a new OIndexException wrapping e1, effectively losing the context of the original NoSuchMethodException e in the thrown exception chain (though e was logged).

Could this be simplified? For instance, if the goal is to try a public no-arg constructor, perhaps classAnalyzer.getDeclaredConstructor().newInstance() would be more direct and modern after classAnalyzer.getConstructor() fails. Also, ensuring the correct exception is wrapped and propagated might improve debuggability.

public static Query createDeleteQuery(
OIdentifiable value, List<String> fields, Object key, ODocument metadata) {

// TODO Implementation of Composite keys with Collection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This TODO comment // TODO Implementation of Composite keys with Collection indicates incomplete functionality for handling composite keys where parts might be collections. What is the plan or timeline for addressing this? This could be a significant limitation depending on use cases.

// TODO Implementation of Composite keys with Collection
final BooleanQuery.Builder filter = new BooleanQuery.Builder();
final BooleanQuery.Builder builder = new BooleanQuery.Builder();
// TODO: Condition on Id and field key only for backward compatibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The TODO comment // TODO: Condition on Id and field key only for backward compatibility suggests this logic might be transitional. Could you elaborate on the backward compatibility concern and the intended future state? Understanding this will help assess the current implementation's suitability.

Comment on lines +186 to +189
} catch (IOException e) {
logger.error("unable to create multi-reader", e);
} catch (ParseException e) {
logger.error("unable to parse query", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Catching IOException and ParseException, logging an error, and then returning null might hide critical issues from the caller. Would it be more appropriate to wrap these exceptions in a domain-specific exception (e.g., ArcadeDBException or a Lucene-specific query exception) and rethrow it? This would allow callers to handle such failures more explicitly.

Copy link

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.04%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (56921db) 72351 46136 63.77%
Head commit (a4e2151) 72352 (+1) 46167 (+31) 63.81% (+0.04%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#2237) 0 0 ∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@robfrank robfrank closed this Jun 3, 2025
@robfrank robfrank deleted the jules_wip_88560451984875179 branch June 3, 2025 08:44
mergify bot added a commit that referenced this pull request Aug 17, 2025
[//]: # (dependabot-start)
⚠️ \*\*Dependabot is rebasing this PR\*\* ⚠️
Rebasing might not happen immediately, so don't worry if this takes some time.
Note: if you make any changes to this PR yourself, they will take precedence over the rebase.
---
[//]: # (dependabot-end)
Bumps [actions/checkout](https://github.com/actions/checkout) from 4.2.2 to 5.0.0.
Release notes

*Sourced from [actions/checkout's releases](https://github.com/actions/checkout/releases).*

> v5.0.0
> ------
>
> What's Changed
> --------------
>
> * Update actions checkout to use node 24 by [`@​salmanmkc`](https://github.com/salmanmkc) in [actions/checkout#2226](https://redirect.github.com/actions/checkout/pull/2226)
> * Prepare v5.0.0 release by [`@​salmanmkc`](https://github.com/salmanmkc) in [actions/checkout#2238](https://redirect.github.com/actions/checkout/pull/2238)
>
> ⚠️ Minimum Compatible Runner Version
> ------------------------------------
>
> **v2.327.1**  
> [Release Notes](https://github.com/actions/runner/releases/tag/v2.327.1)
>
> Make sure your runner is updated to this version or newer to use this release.
>
> **Full Changelog**: <actions/checkout@v4...v5.0.0>
>
> v4.3.0
> ------
>
> What's Changed
> --------------
>
> * docs: update README.md by [`@​motss`](https://github.com/motss) in [actions/checkout#1971](https://redirect.github.com/actions/checkout/pull/1971)
> * Add internal repos for checking out multiple repositories by [`@​mouismail`](https://github.com/mouismail) in [actions/checkout#1977](https://redirect.github.com/actions/checkout/pull/1977)
> * Documentation update - add recommended permissions to Readme by [`@​benwells`](https://github.com/benwells) in [actions/checkout#2043](https://redirect.github.com/actions/checkout/pull/2043)
> * Adjust positioning of user email note and permissions heading by [`@​joshmgross`](https://github.com/joshmgross) in [actions/checkout#2044](https://redirect.github.com/actions/checkout/pull/2044)
> * Update README.md by [`@​nebuk89`](https://github.com/nebuk89) in [actions/checkout#2194](https://redirect.github.com/actions/checkout/pull/2194)
> * Update CODEOWNERS for actions by [`@​TingluoHuang`](https://github.com/TingluoHuang) in [actions/checkout#2224](https://redirect.github.com/actions/checkout/pull/2224)
> * Update package dependencies by [`@​salmanmkc`](https://github.com/salmanmkc) in [actions/checkout#2236](https://redirect.github.com/actions/checkout/pull/2236)
> * Prepare release v4.3.0 by [`@​salmanmkc`](https://github.com/salmanmkc) in [actions/checkout#2237](https://redirect.github.com/actions/checkout/pull/2237)
>
> New Contributors
> ----------------
>
> * [`@​motss`](https://github.com/motss) made their first contribution in [actions/checkout#1971](https://redirect.github.com/actions/checkout/pull/1971)
> * [`@​mouismail`](https://github.com/mouismail) made their first contribution in [actions/checkout#1977](https://redirect.github.com/actions/checkout/pull/1977)
> * [`@​benwells`](https://github.com/benwells) made their first contribution in [actions/checkout#2043](https://redirect.github.com/actions/checkout/pull/2043)
> * [`@​nebuk89`](https://github.com/nebuk89) made their first contribution in [actions/checkout#2194](https://redirect.github.com/actions/checkout/pull/2194)
> * [`@​salmanmkc`](https://github.com/salmanmkc) made their first contribution in [actions/checkout#2236](https://redirect.github.com/actions/checkout/pull/2236)
>
> **Full Changelog**: <actions/checkout@v4...v4.3.0>


Changelog

*Sourced from [actions/checkout's changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md).*

> Changelog
> =========
>
> V5.0.0
> ------
>
> * Update actions checkout to use node 24 by [`@​salmanmkc`](https://github.com/salmanmkc) in [actions/checkout#2226](https://redirect.github.com/actions/checkout/pull/2226)
>
> V4.3.0
> ------
>
> * docs: update README.md by [`@​motss`](https://github.com/motss) in [actions/checkout#1971](https://redirect.github.com/actions/checkout/pull/1971)
> * Add internal repos for checking out multiple repositories by [`@​mouismail`](https://github.com/mouismail) in [actions/checkout#1977](https://redirect.github.com/actions/checkout/pull/1977)
> * Documentation update - add recommended permissions to Readme by [`@​benwells`](https://github.com/benwells) in [actions/checkout#2043](https://redirect.github.com/actions/checkout/pull/2043)
> * Adjust positioning of user email note and permissions heading by [`@​joshmgross`](https://github.com/joshmgross) in [actions/checkout#2044](https://redirect.github.com/actions/checkout/pull/2044)
> * Update README.md by [`@​nebuk89`](https://github.com/nebuk89) in [actions/checkout#2194](https://redirect.github.com/actions/checkout/pull/2194)
> * Update CODEOWNERS for actions by [`@​TingluoHuang`](https://github.com/TingluoHuang) in [actions/checkout#2224](https://redirect.github.com/actions/checkout/pull/2224)
> * Update package dependencies by [`@​salmanmkc`](https://github.com/salmanmkc) in [actions/checkout#2236](https://redirect.github.com/actions/checkout/pull/2236)
>
> v4.2.2
> ------
>
> * `url-helper.ts` now leverages well-known environment variables by [`@​jww3`](https://github.com/jww3) in [actions/checkout#1941](https://redirect.github.com/actions/checkout/pull/1941)
> * Expand unit test coverage for `isGhes` by [`@​jww3`](https://github.com/jww3) in [actions/checkout#1946](https://redirect.github.com/actions/checkout/pull/1946)
>
> v4.2.1
> ------
>
> * Check out other refs/\* by commit if provided, fall back to ref by [`@​orhantoy`](https://github.com/orhantoy) in [actions/checkout#1924](https://redirect.github.com/actions/checkout/pull/1924)
>
> v4.2.0
> ------
>
> * Add Ref and Commit outputs by [`@​lucacome`](https://github.com/lucacome) in [actions/checkout#1180](https://redirect.github.com/actions/checkout/pull/1180)
> * Dependency updates by [`@​dependabot`](https://github.com/dependabot)- [actions/checkout#1777](https://redirect.github.com/actions/checkout/pull/1777), [actions/checkout#1872](https://redirect.github.com/actions/checkout/pull/1872)
>
> v4.1.7
> ------
>
> * Bump the minor-npm-dependencies group across 1 directory with 4 updates by [`@​dependabot`](https://github.com/dependabot) in [actions/checkout#1739](https://redirect.github.com/actions/checkout/pull/1739)
> * Bump actions/checkout from 3 to 4 by [`@​dependabot`](https://github.com/dependabot) in [actions/checkout#1697](https://redirect.github.com/actions/checkout/pull/1697)
> * Check out other refs/\* by commit by [`@​orhantoy`](https://github.com/orhantoy) in [actions/checkout#1774](https://redirect.github.com/actions/checkout/pull/1774)
> * Pin actions/checkout's own workflows to a known, good, stable version. by [`@​jww3`](https://github.com/jww3) in [actions/checkout#1776](https://redirect.github.com/actions/checkout/pull/1776)
>
> v4.1.6
> ------
>
> * Check platform to set archive extension appropriately by [`@​cory-miller`](https://github.com/cory-miller) in [actions/checkout#1732](https://redirect.github.com/actions/checkout/pull/1732)
>
> v4.1.5
> ------
>
> * Update NPM dependencies by [`@​cory-miller`](https://github.com/cory-miller) in [actions/checkout#1703](https://redirect.github.com/actions/checkout/pull/1703)
> * Bump github/codeql-action from 2 to 3 by [`@​dependabot`](https://github.com/dependabot) in [actions/checkout#1694](https://redirect.github.com/actions/checkout/pull/1694)
> * Bump actions/setup-node from 1 to 4 by [`@​dependabot`](https://github.com/dependabot) in [actions/checkout#1696](https://redirect.github.com/actions/checkout/pull/1696)
> * Bump actions/upload-artifact from 2 to 4 by [`@​dependabot`](https://github.com/dependabot) in [actions/checkout#1695](https://redirect.github.com/actions/checkout/pull/1695)
> * README: Suggest `user.email` to be `41898282+github-actions[bot]@users.noreply.github.com` by [`@​cory-miller`](https://github.com/cory-miller) in [actions/checkout#1707](https://redirect.github.com/actions/checkout/pull/1707)
>
> v4.1.4
> ------
>
> * Disable `extensions.worktreeConfig` when disabling `sparse-checkout` by [`@​jww3`](https://github.com/jww3) in [actions/checkout#1692](https://redirect.github.com/actions/checkout/pull/1692)
> * Add dependabot config by [`@​cory-miller`](https://github.com/cory-miller) in [actions/checkout#1688](https://redirect.github.com/actions/checkout/pull/1688)
> * Bump the minor-actions-dependencies group with 2 updates by [`@​dependabot`](https://github.com/dependabot) in [actions/checkout#1693](https://redirect.github.com/actions/checkout/pull/1693)
> * Bump word-wrap from 1.2.3 to 1.2.5 by [`@​dependabot`](https://github.com/dependabot) in [actions/checkout#1643](https://redirect.github.com/actions/checkout/pull/1643)
>
> v4.1.3
> ------

... (truncated)


Commits

* [`08c6903`](actions/checkout@08c6903) Prepare v5.0.0 release ([#2238](https://redirect.github.com/actions/checkout/issues/2238))
* [`9f26565`](actions/checkout@9f26565) Update actions checkout to use node 24 ([#2226](https://redirect.github.com/actions/checkout/issues/2226))
* [`08eba0b`](actions/checkout@08eba0b) Prepare release v4.3.0 ([#2237](https://redirect.github.com/actions/checkout/issues/2237))
* [`631c7dc`](actions/checkout@631c7dc) Update package dependencies ([#2236](https://redirect.github.com/actions/checkout/issues/2236))
* [`8edcb1b`](actions/checkout@8edcb1b) Update CODEOWNERS for actions ([#2224](https://redirect.github.com/actions/checkout/issues/2224))
* [`09d2aca`](actions/checkout@09d2aca) Update README.md ([#2194](https://redirect.github.com/actions/checkout/issues/2194))
* [`85e6279`](actions/checkout@85e6279) Adjust positioning of user email note and permissions heading ([#2044](https://redirect.github.com/actions/checkout/issues/2044))
* [`009b9ae`](actions/checkout@009b9ae) Documentation update - add recommended permissions to Readme ([#2043](https://redirect.github.com/actions/checkout/issues/2043))
* [`cbb7224`](actions/checkout@cbb7224) Update README.md ([#1977](https://redirect.github.com/actions/checkout/issues/1977))
* [`3b9b8c8`](actions/checkout@3b9b8c8) docs: update README.md ([#1971](https://redirect.github.com/actions/checkout/issues/1971))
* See full diff in [compare view](actions/checkout@11bd719...08c6903)
  
[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility\_score?dependency-name=actions/checkout&package-manager=github\_actions&previous-version=4.2.2&new-version=5.0.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
Dependabot commands and options
  
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot show  ignore conditions` will show all of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant