Skip to content

General codebase fixes#293

Closed
hrstoyanov wants to merge 37 commits intoArcadeData:mainfrom
hrstoyanov:fixes
Closed

General codebase fixes#293
hrstoyanov wants to merge 37 commits intoArcadeData:mainfrom
hrstoyanov:fixes

Conversation

@hrstoyanov
Copy link

What does this PR do?
I ran ArcadeDB through Intellij code inspection and got back a lot of issues.

Motivation
Wanted to improve/modernize the code base.

I did not accept all fixes IntelliJ offered. For example, some of them are related to generics and require more investigation.

Checklist
[X] I have run the build using mvn clean package command
[X] My unit tests cover both failure and success scenarios

@lvca
Copy link
Member

lvca commented Jan 25, 2022

Hi @hrstoyanov, thanks for your PR! I quickly looked at some of the commits contained in the PR and they are legit. I can't merge the entire PR because it contains some commits I prefer not to include, like the first commit that undoes some changes you did in your branch but are not relevant in the main codebase.

Also, I see you fixed some issues in the SQL and GraphQL classes, but that code is automatically generated by JavaCC parser every time we modify the SQL/GraphQL grammars, so they would be lost at the very next change to the grammar.

I'm going to check one by one commit and cherry-pick the relevant ones.

@hrstoyanov
Copy link
Author

Thanks Luka. Please cherry pick the commits - this is the best approach. I did try to avoid fixing auto-genetated classes, but I missed - please ignore them.

I did not realize how big of PR it is, so it's best to cherry pick and do this in stages.

@hrstoyanov
Copy link
Author

A few things I can try once you are done with absorbing this batch:

  • there were a few fixes I could not understand why IntelliJ suggested. I can try them again.
  • I see the code uses SecurityManager which is to be removed from Java soon (jep 411).
  • I see usage of Unsafe for comparisons?
  • Generics. This could be sizable work.

Copy link
Member

@lvca lvca left a comment

Choose a reason for hiding this comment

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

Thanks @hrstoyanov, your contribution is much appreciated! I've finished my one by one commi review and I've already merged most of them. Thanks! I see you reverted the last commit but there were nice changes. WDYT about getting a rebase from main and see if you can send more commits by the topic of change?


@Override
protected void handleCorruption(final Exception e, final RID edge, final RID vertex) {
if ((e instanceof RecordNotFoundException || e instanceof SchemaException) &&//
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this exception catching?

Copy link
Author

Choose a reason for hiding this comment

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

@lvca I always start from the latest code in your repo. I will wait for you to finish picking changes for the last 2 PRs, and continue from the latest code base.

@hrstoyanov hrstoyanov changed the title Genetral codebase fixes General codebase fixes Jan 27, 2022
@hrstoyanov hrstoyanov marked this pull request as draft January 27, 2022 08:24
@lvca lvca self-assigned this Jan 29, 2022
@lvca lvca added this to the 22.1.3 milestone Jan 29, 2022
@lvca lvca added the enhancement New feature or request label Jan 29, 2022
lvca added a commit that referenced this pull request Jan 31, 2022
@lvca
Copy link
Member

lvca commented Feb 1, 2022

Merged single commits.

@lvca lvca closed this Feb 1, 2022
lvca added a commit that referenced this pull request Dec 23, 2022
lvca added a commit that referenced this pull request Dec 23, 2022
* First working implementation and test cases. Only for the engine. Still missing docs, integration with studio, storing on file and HTTP support.

Issue #288

* User function: managed timeout on script execution

This prevent infinite loops by mistake and on purpose intended as an attack

* js engine: Added another test for the sandbox

* Supported Java query engine. This invokes a Java method. Nice to write custom function easy to debug when runs embedded

* Custom function: added security registration of allowed classes

This avoid to use the java query engine to execute any Java method, but only from the classes registrated beforehand

* Hava query engine: improved using of underlying exception

* Refactoring to let the user to register the functions with a specific API

* Complete refactoring of the function API

To have libraries with functions written in different languages. With fluent API it's easier to declare the functions. Also, now functions are registered on a central place, so we could call them from SQL too.

* SQL: merged SQLEngine class into SQLQueryEngine. Now SQL can see functions defined in libraries

* Added javadoc on Database interface + minor changes to API

* Implemented suggestions by @hrstoyanov

#293

* Fixed Java engine supported languages

* Fixed typo with Javadoc

* Fixed compilation problems after rebase

* Fixed compilation errors after rebase

* fix: resolved merge issues

* Fix issue with caching functions

* feat: functions can be managed from SQL

Issue #288 task 5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants