-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Java: VSS commands init #2385
Java: VSS commands init #2385
Conversation
@@ -165,8 +165,8 @@ jar.dependsOn('copyNativeLib') | |||
javadoc.dependsOn('copyNativeLib') | |||
copyNativeLib.dependsOn('buildRustRelease') | |||
compileTestJava.dependsOn('copyNativeLib') | |||
test.dependsOn('buildRust') | |||
testFfi.dependsOn('buildRust') | |||
test.dependsOn('buildRustRelease') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? I thought we were okay with running tests vs the debug version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gradle still runs UT on release due to copyNativeLib
. At least it doesn't build debug version which is never used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know when we decided to switch to the release version. Better to run against debug, but we can fix this in a separate PR
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@@ -685,6 +691,9 @@ impl RequestType { | |||
RequestType::ScriptExists => Some(get_two_word_command("SCRIPT", "EXISTS")), | |||
RequestType::ScriptFlush => Some(get_two_word_command("SCRIPT", "FLUSH")), | |||
RequestType::ScriptKill => Some(get_two_word_command("SCRIPT", "KILL")), | |||
RequestType::FtCreate => Some(cmd("FT.CREATE")), | |||
RequestType::FtSearch => Some(cmd("FT.SEARCH")), | |||
RequestType::FtDrop => Some(cmd("FT.DROPINDEX")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequestType::FtDrop => Some(cmd("FT.DROPINDEX")), | |
RequestType::FtDropIndex => Some(cmd("FT.DROPINDEX")), |
@@ -457,6 +460,9 @@ impl From<::protobuf::EnumOrUnknown<ProtobufRequestType>> for RequestType { | |||
ProtobufRequestType::ScriptFlush => RequestType::ScriptFlush, | |||
ProtobufRequestType::ScriptKill => RequestType::ScriptKill, | |||
ProtobufRequestType::ScriptShow => RequestType::ScriptShow, | |||
ProtobufRequestType::FtCreate => RequestType::FtCreate, | |||
ProtobufRequestType::FtSearch => RequestType::FtSearch, | |||
ProtobufRequestType::FtDrop => RequestType::FtDrop, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProtobufRequestType::FtDrop => RequestType::FtDrop, | |
ProtobufRequestType::FtDropIndex => RequestType::FtDropIndex, |
@@ -227,6 +227,9 @@ pub enum RequestType { | |||
ScriptFlush = 216, | |||
ScriptKill = 217, | |||
ScriptShow = 218, | |||
FtCreate = 2000, | |||
FtSearch = 2001, | |||
FtDrop = 2002, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FtDrop = 2002, | |
FtDropIndex = 2002, |
|
||
FtCreate = 2000; | ||
FtSearch = 2001; | ||
FtDrop = 2002; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FtDrop = 2002; | |
FtDropIndex = 2002; |
@@ -165,8 +165,8 @@ jar.dependsOn('copyNativeLib') | |||
javadoc.dependsOn('copyNativeLib') | |||
copyNativeLib.dependsOn('buildRustRelease') | |||
compileTestJava.dependsOn('copyNativeLib') | |||
test.dependsOn('buildRust') | |||
testFfi.dependsOn('buildRust') | |||
test.dependsOn('buildRustRelease') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know when we decided to switch to the release version. Better to run against debug, but we can fix this in a separate PR
import glide.api.models.commands.vss.FTSearchOptions.FTSearchOptionsBuilder; | ||
import java.util.concurrent.CompletableFuture; | ||
|
||
public interface VectorSearchBaseCommands { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this to a Static Public Class and put it under package glide.api.commands.servermodules.
Move the commands function from BaseClient to here (add the argument BaseClient to each command).
We don't need to separate Interface and commands, and we can put the documentation in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the JSON and FT commands, we should rename this class to FT
, and commands should only reference the action. Example:
import static glide.api.models.commands. servermodules.FT;
FT.create(client, "hash_idx1", FTCreateOptions.empty(), new FieldInfo[] {
new FieldInfo("vec", VectorFieldFlat.builder(DistanceMetric.L2, 2).build())
}).get();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do after merging #2414
* client.ftdrop("hash_idx1").get(); | ||
* }</pre> | ||
*/ | ||
CompletableFuture<String> ftdrop(String indexName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CompletableFuture<String> ftdrop(String indexName); | |
CompletableFuture<String> ftdropindex(String indexName); |
var args = new ArrayList<String>(); | ||
args.add("TAG"); | ||
if (separator.isPresent()) { | ||
args.add("SEPARATOR"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these keywords be constants?
@Override | ||
public String[] toArgs() { | ||
var args = new ArrayList<String>(); | ||
args.add("VECTOR"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Types TEXT, VECTOR, TAG, etc. could be Enums.
var args = new ArrayList<String>(); | ||
args.add("VECTOR"); | ||
args.add(Algorithm); | ||
args.add(Integer.toString(params.size() * 2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no null check for params? This is a potential null pointer exception.
*/ | ||
public static class VectorFieldHnsw extends VectorField { | ||
protected VectorFieldHnsw(Map<String, String> params) { | ||
super(params, "HNSW"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"HNSW" value can be taken from Algorithm Enum.
concatenateArrays( | ||
new GlideString[] {gs(indexName), gs(query)}, | ||
options.toArgs(), | ||
new GlideString[] {gs("DIALECT"), gs("2")}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this option in the inputs.
FT.CREATE
FT.SEARCH
FT.DROP