fix: build warnings and test failures (#3457)#3465
Conversation
robfrank
commented
Feb 17, 2026
robfrank
commented
- Add missing TYPE_BINARY -> byte[].class mapping in BinaryTypes.getClassFromType(), fixing BinaryIndexTest and LSMTreeIndexTest failures after database reopen
- Fix LSMVectorIndex graph storage: create graphFile placeholder when no .vecgraph exists on disk, and reload graph as OnDiskGraphIndex after persistence so the current session benefits from inline vector storage immediately
- Add explicit (String[]) null casts to resolve varargs compiler warnings in CheckDatabaseTest and RemoteDatabaseIT
- Remove unused proto imports (empty.proto, struct.proto) from arcadedb-server.proto
- Remove unused test-jar goal from e2e and e2e-perf modules
- Add propertiesEncoding to maven-resources-plugin in root POM
Summary of ChangesHello @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! This pull request focuses on improving the stability and cleanliness of the ArcadeDB project by addressing several build-related issues. It fixes critical test failures related to binary type handling and refines the behavior of the LSMVectorIndex for better graph persistence and immediate availability of inline vector storage. Additionally, it resolves compiler warnings and streamlines the build configuration by removing unnecessary elements and ensuring proper encoding for resources. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
🧪 CI InsightsHere's what we observed from your CI run for 549ee5c. 🟢 All jobs passed!But CI Insights is watching 👀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides a good set of fixes for build warnings, test failures, and other small improvements across the codebase. The changes are well-described and correctly implemented. Key improvements include fixing a bug in binary type mapping, resolving varargs compiler warnings, and enhancing the LSMVectorIndex to be more robust and performant by creating graph file placeholders and immediately reloading the graph after persistence. The POM file cleanups and configuration enhancements are also valuable. Overall, this is a solid contribution that improves the stability and quality of the project.
Code ReviewOverall this is a well-targeted fix PR addressing build warnings, test failures, and a real correctness issue in the vector index. Here is my analysis of each change: BinaryTypes.java — Missing TYPE_BINARY mappingPositive. The missing LSMVectorIndex.java — Placeholder graph file + immediate reloadConcern: Placeholder creation creates an actual file on disk In the "load existing index" constructor, the new this.graphFile = new LSMVectorIndexGraphFile(database, graphFileName, graphFilePath, mode, pageSize);
database.getSchema().getEmbedded().registerFile(this.graphFile);The
The comment says "placeholder" but it is actually a real (empty) file on disk. Please verify the read-only mode path. Positive: Immediate OnDiskGraphIndex reload The change to immediately reload the graph as Minor nit: the exception handling only logs LogManager.instance().log(this, Level.WARNING,
"Could not reload graph as OnDiskGraphIndex (will use in-memory graph): %s", e.getMessage());Consider passing Varargs null casts (CheckDatabaseTest.java, RemoteDatabaseIT.java)Correct. The explicit Protobuf — Remove unused importsPositive. Removing unused Build — Remove test-jar goals (e2e/pom.xml, e2e-perf/pom.xml)Positive. Test-jar artifacts from e2e modules are not consumed anywhere, so removing them is correct. Root POM — propertiesEncodingPositive. Adding Summary
The one item worth validating before merge is the read-only mode scenario for the placeholder file creation in the "load existing index" constructor. |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3465 +/- ##
==========================================
- Coverage 62.90% 62.83% -0.08%
==========================================
Files 1297 1299 +2
Lines 85969 86378 +409
Branches 17668 17767 +99
==========================================
+ Hits 54082 54274 +192
- Misses 24183 24333 +150
- Partials 7704 7771 +67 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code ReviewOverall, this PR is a solid set of bug fixes and maintenance improvements. The changes are focused, well-reasoned, and address real test failures. Here are my observations: BinaryTypes.java – Missing TYPE_BINARY mapping (Engine)Good fix. Adding LSMVectorIndex.java – Graph file placeholder on loadConcern: double-registration risk. In the loading constructor, when The subtler question: is it safe to register a file that has no on-disk content yet? The file ID will be allocated, and if the database closes before Minor: The LSMVectorIndex.java – Immediate reload as OnDiskGraphIndexGood improvement over the previous workaround comment. Reloading the graph inline after persistence means the current session benefits from inline vector storage without requiring a restart. Concern: exception swallowing. The catch block at lines 1417-1420 logs a WARNING and continues silently. This is intentional (the comment says "will use in-memory graph") and is acceptable for a best-effort optimization. However, the warning message only logs Varargs cast fixes in test filesCorrect. The explicit Protobuf / gRPC cleanupGood housekeeping.
Build / POM changes
Summary
The main item worth a second look is confirming that a registered-but-empty |
|
+1! |
robfrank
force-pushed
the
fix/3457-build-warnings-test-failures
branch
from
536153d to
18fe15b
Compare
Code ReviewThis PR is a solid set of targeted fixes addressing build warnings, test failures, and index robustness. Here are my observations:
|
Code ReviewOverall, this PR is a solid cleanup that fixes real bugs and build warnings. Here is my analysis of each change: BinaryTypes.java — Missing TYPE_BINARY mappingPositive: This is a genuine bug fix. The missing Question: Are there any other missing mappings worth auditing? It may be worth a follow-up to ensure every LSMVectorIndex.java — Graph file placeholder creationConcern: The change creates a new
Suggestion: Consider adding a comment explaining the lifecycle — that the placeholder is "lazy" and no file is created on disk until LSMVectorIndex.java — Immediate reload as OnDiskGraphIndexThe previous comment in the code was explicit about why the reload was deferred: This PR removes that reasoning and reloads immediately. Concern: The catch block swallows the exception and logs only Minor: Is there a test that verifies the CheckDatabaseTest.java and RemoteDatabaseIT.java — varargs null fixesUsing the correct zero/typed-arg overload instead of passing Proto file — Remove unused importsRemoving pom.xml — protobuf-maven-plugin upgrade (4.1.3 to 5.0.0)The plugin configuration syntax changed ( pom.xml — propertiesEncoding additionAdding e2e/pom.xml and e2e-perf/pom.xml — Remove unused test-jar goalsGood housekeeping. Verify nothing in the build references these test JARs as dependencies (a grep for grpc/pom.xml — Whitespace normalizationPure formatting (tabs to spaces). Fine, but creates noise in diffs. Summary
No blocking issues, but the |
Code ReviewThis PR addresses several important build warnings and test failures. The changes are generally well-targeted and focused. Here are my observations:
|
| Area | Assessment |
|---|---|
BinaryTypes bug fix |
✅ Correct fix |
| Vector index placeholder | |
| Immediate graph reload | |
| Security audit disabled | |
| Proto/Maven/build cleanup | ✅ All good |
| Varargs warning fixes | ✅ Clean |
The core bug fixes are solid. The main areas needing attention before merge are:
- Clarify whether creating a
LSMVectorIndexGraphFileplaceholder always registers/creates a file on disk for new indexes that haven't built a graph yet. - Explain why the "page visibility after commit" concern from the removed comment no longer applies.
- Document the reason for disabling npm security auditing, or restore it with the specific exception properly handled.
Code ReviewOverall this is a solid bug-fix / housekeeping PR. The individual changes are well-scoped, and the root causes are clearly explained. A few things worth discussing:
|
| Area | Verdict |
|---|---|
BinaryTypes mapping fix |
✅ Good |
| Varargs/null-cast fixes | ✅ Good (description slightly off) |
| Proto cleanup | ✅ Good |
| POM cleanups | ✅ Good |
LSMVectorIndex lazy graph file |
|
| Studio security audit disabled |
|
This is huge improvement already. Almost all relevant warnings are gone, and only one failure in the engine module tests remain in MacOS and (Ubuntu) Linux respectively. Gremlin and e2e module tests still fail (the former only MacOS, the latter on MacOS and Ubuntu but differently). I updated the associated issue with the remaining problems: #3457 |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Code Review: PR #3465 — fix: build warnings and test failures (#3457)This PR is a clean-up/fix PR addressing build warnings, compiler errors, and test failures. The changes are mostly mechanical but contain one substantive bug fix and one behavioural change worth discussing. ✅ Positives
|
| Area | Verdict |
|---|---|
| Import cleanup | ✅ Correct and follows project conventions |
BinaryTypes bug fix |
✅ Correct fix |
getOrCreateGraphFile() lazy init |
✅ Good approach, minor concern about silent null |
Graph reload as OnDiskGraphIndex |
✅ Good, needs commit-precondition comment |
| npm audit disabled | |
| Wildcard imports in tests | 🔵 Minor style inconsistency |
grpc/pom.xml tab→space reformat |
🔵 Noisy diff, low risk |
The PR is generally safe to merge after addressing the npm audit concern and adding comments around the graph persistence fallback behaviour.
Code ReviewOverall this is a solid cleanup PR. The changes are well-scoped and address real issues. Below are observations ranging from concerns to minor style notes. Bug Fix:
|
| Area | Verdict |
|---|---|
BinaryTypes fix |
Correct and necessary |
getOrCreateGraphFile() thread safety |
Concern — should hold write lock |
Immediate OnDiskGraphIndex reload |
Needs rationale for reverting prior decision |
count(null) → count() semantic change |
Needs confirmation of equivalence |
protobuf-maven-plugin v5 upgrade |
Looks correct, verify embedSourcesInClassOutputs |
throws Exception widening in tests |
Minor — prefer throws IOException |
| Import cleanup, test renames | All good |
| Wildcard imports | Minor — prefer explicit imports per project style |
Code ReviewThis PR addresses build warnings and test failures, with the most significant functional changes being the BinaryTypes.java — Missing TYPE_BINARY Mapping ✅The fix is correct and minimal. The missing DefaultLogger.java — Unnecessary Text Block Usage
|
| Change | Assessment |
|---|---|
BinaryTypes TYPE_BINARY fix |
✅ Correct and necessary |
LSMVectorIndex lazy graph + immediate reload |
✅ Good; verify thread safety of non-final graphFile |
DefaultLogger text blocks for string constants |
|
CypherASTBuilder regex as text block |
|
CheckDatabaseTest count() change |
✅ Clean fix |
| Text block conversions in log messages | ✅ Mostly fine; check leading-space indentation |
| Wildcard imports | |
| Build and import cleanup | ✅ Clean |
Code ReviewThis PR fixes several legitimate issues (build warnings, test failures, Issues1.
|
Code ReviewThis PR addresses a solid set of build warnings, test failures, and code quality improvements. Bug Fixes
The addition of
The approach is sound: instead of eagerly creating an empty One concern: Code QualityWildcard imports in Both files now use Text-block conversions Replacing multi-line string concatenation with text blocks is welcome. The indentation stripping is correct (closing Test method signatures: The npm security-check audit level change in Changing Proto file cleanup Removing the unused
A good defensive addition to ensure Removal of unused Correct. These modules have no consumers of their test jars. SummaryThe core bug fixes ( Items worth addressing before merge:
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
- Add missing TYPE_BINARY -> byte[].class mapping in BinaryTypes.getClassFromType(),
fixing BinaryIndexTest and LSMTreeIndexTest failures after database reopen
- Fix LSMVectorIndex graph storage: create graphFile placeholder when no .vecgraph
exists on disk, and reload graph as OnDiskGraphIndex after persistence so the
current session benefits from inline vector storage immediately
- Add explicit (String[]) null casts to resolve varargs compiler warnings in
CheckDatabaseTest and RemoteDatabaseIT
- Remove unused proto imports (empty.proto, struct.proto) from arcadedb-server.proto
- Remove unused test-jar goal from e2e and e2e-perf modules
- Add propertiesEncoding to maven-resources-plugin in root POM
robfrank
force-pushed
the
fix/3457-build-warnings-test-failures
branch
from
c4dc99d to
549ee5c
Compare
PR ReviewOverall this is a solid cleanup PR. The bug fixes are correct and the formatting improvements make the code more readable. Here are my observations: Bug Fixes (Correct and Well-Done)
The added
The introduction of
Issues and ConcernsThread-Safety:
|
| Area | Assessment |
|---|---|
BinaryTypes bug fix |
Correct |
LSMVectorIndex graph persistence |
Improvement, but needs thread-safety review |
| Varargs null cast fixes | Correct |
| Wildcard import in test | Inconsistent with project style |
| Text blocks on single-line strings | Cosmetic concern |
protobuf-maven-plugin major version bump |
Verify CI builds cleanly |
The thread-safety concern with the non-final graphFile field is the most important issue to address before merging.
* fix and chore: build warnings and test failures (#3457) - Add missing TYPE_BINARY -> byte[].class mapping in BinaryTypes.getClassFromType(), fixing BinaryIndexTest and LSMTreeIndexTest failures after database reopen - Fix LSMVectorIndex graph storage: create graphFile placeholder when no .vecgraph exists on disk, and reload graph as OnDiskGraphIndex after persistence so the current session benefits from inline vector storage immediately - Add explicit (String[]) null casts to resolve varargs compiler warnings in CheckDatabaseTest and RemoteDatabaseIT - Remove unused proto imports (empty.proto, struct.proto) from arcadedb-server.proto - Remove unused test-jar goal from e2e and e2e-perf modules - Add propertiesEncoding to maven-resources-plugin in root POM - upgrade protobuf-maven-plugin to v5.0.0 and update configuration - fix: mitigate npm audit - fix: remove fqns - fix: migrate to assertJ all JUnit asserts - chore: remove some string concatenation (cherry picked from commit 5ebbdb7)
3 participants
