Skip to content
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

Closed
wants to merge 18 commits into from
Closed
67 changes: 63 additions & 4 deletions .github/workflows/java.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ concurrency:
group: java-${{ github.head_ref || github.ref }}
cancel-in-progress: true

permissions:
contents: read
# Allows the GITHUB_TOKEN to make an API call to generate an OIDC token.
id-token: write

jobs:
load-engine-matrix:
runs-on: ubuntu-latest
Expand Down Expand Up @@ -57,10 +62,10 @@ jobs:
engine: ${{ fromJson(needs.load-engine-matrix.outputs.matrix) }}
host:
- {
OS: ubuntu,
RUNNER: ubuntu-latest,
TARGET: x86_64-unknown-linux-gnu
}
OS: ubuntu,
RUNNER: ubuntu-latest,
TARGET: x86_64-unknown-linux-gnu,
}
# - {
# OS: macos,
# RUNNER: macos-latest,
Expand Down Expand Up @@ -195,3 +200,57 @@ jobs:
with:
cargo-toml-folder: ./java
name: lint java rust

# start-self-hosted-runner:
# if: github.repository_owner == 'valkey-io'
# runs-on: ubuntu-latest
# environment: AWS_ACTIONS
# steps:
# - name: Checkout
# uses: actions/checkout@v4
#
# - name: Start self hosted EC2 runner
# uses: ./.github/workflows/start-self-hosted-runner
# with:
# role-to-assume: ${{ secrets.ROLE_TO_ASSUME }}
# aws-region: ${{ secrets.AWS_REGION }}
# ec2-instance-id: ${{ secrets.AWS_EC2_INSTANCE_ID }}

test-modules:
if: github.repository_owner == 'valkey-io'
# needs: [start-self-hosted-runner]
name: Running Module Tests
runs-on: [self-hosted, linux, ARM64]
timeout-minutes: 15
steps:
- name: Setup self-hosted runner access
run: sudo chown -R $USER:$USER /home/ubuntu/actions-runner/_work/valkey-glide

- uses: actions/checkout@v4
with:
submodules: recursive

- name: Set up JDK
uses: actions/setup-java@v4
with:
distribution: "temurin"
java-version: 17

- name: Install protoc (protobuf)
uses: arduino/setup-protoc@v3
with:
version: "26.1"
repo-token: ${{ secrets.GITHUB_TOKEN }}

- name: Test java wrapper
working-directory: java
run: ./gradlew :integTest:modulesTest -Dcluster-endpoints=${{ secrets.MEMDB_MODULES_ENDPOINT }} -Dstandalone-endpoints= -Dtls=true
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved

- name: Upload test reports
if: always()
continue-on-error: true
uses: actions/upload-artifact@v4
with:
name: test-reports-modules
path: |
java/integTest/build/reports/**
50 changes: 49 additions & 1 deletion glide-core/src/client/value_conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub(crate) enum ExpectedReturnType<'a> {
ArrayOfStrings,
ArrayOfBools,
ArrayOfDoubleOrNull,
FTSearchReturnType,
Lolwut,
ArrayOfStringAndArrays,
ArrayOfArraysOfDoubleOrNull,
Expand Down Expand Up @@ -891,7 +892,53 @@ pub(crate) fn convert_to_expected_type(
format!("(response was {:?})", get_value_type(&value)),
)
.into()),
}
},
ExpectedReturnType::FTSearchReturnType => match value {
/*
Example of the response
1) (integer) 2
2) "json:2"
3) 1) "__VEC_score"
2) "11.1100006104"
3) "$"
4) "{\"vec\":[1.1,1.2,1.3,1.4,1.5,1.6]}"
4) "json:0"
5) 1) "__VEC_score"
2) "91"
3) "$"
4) "{\"vec\":[1,2,3,4,5,6]}"

Converting response to
1) (integer) 2
2) 1# "json:2" =>
1# "__VEC_score" => "11.1100006104"
2# "$" => "{\"vec\":[1.1,1.2,1.3,1.4,1.5,1.6]}"
2# "json:0" =>
1# "__VEC_score" => "91"
2# "$" => "{\"vec\":[1,2,3,4,5,6]}"

Response may contain only 1 element, no conversion in that case.
*/
Value::Array(ref array) if array.len() == 1 => Ok(value),
Value::Array(mut array) => {
Ok(Value::Array(vec![
array.remove(0),
convert_to_expected_type(Value::Array(array), Some(ExpectedReturnType::Map {
key_type: &Some(ExpectedReturnType::BulkString),
value_type: &Some(ExpectedReturnType::Map {
key_type: &Some(ExpectedReturnType::BulkString),
value_type: &Some(ExpectedReturnType::BulkString),
}),
}))?
]))
},
_ => Err((
ErrorKind::TypeError,
"Response couldn't be converted to Pair",
format!("(response was {:?})", get_value_type(&value)),
)
.into())
},
}
}

Expand Down Expand Up @@ -1256,6 +1303,7 @@ pub(crate) fn expected_type_for_cmd(cmd: &Cmd) -> Option<ExpectedReturnType> {
key_type: &None,
value_type: &None,
}),
b"FT.SEARCH" => Some(ExpectedReturnType::FTSearchReturnType),
_ => None,
}
}
Expand Down
4 changes: 4 additions & 0 deletions glide-core/src/protobuf/command_request.proto
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,10 @@ enum RequestType {
ScriptFlush = 216;
ScriptKill = 217;
ScriptShow = 218;

FtCreate = 2000;
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
FtSearch = 2001;
FtDrop = 2002;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FtDrop = 2002;
FtDropIndex = 2002;

}

message Command {
Expand Down
9 changes: 9 additions & 0 deletions glide-core/src/request_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,9 @@ pub enum RequestType {
ScriptFlush = 216,
ScriptKill = 217,
ScriptShow = 218,
FtCreate = 2000,
FtSearch = 2001,
FtDrop = 2002,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FtDrop = 2002,
FtDropIndex = 2002,

}

fn get_two_word_command(first: &str, second: &str) -> Cmd {
Expand Down Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ProtobufRequestType::FtDrop => RequestType::FtDrop,
ProtobufRequestType::FtDropIndex => RequestType::FtDropIndex,

}
}
}
Expand Down Expand Up @@ -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")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RequestType::FtDrop => Some(cmd("FT.DROPINDEX")),
RequestType::FtDropIndex => Some(cmd("FT.DROPINDEX")),

}
}
}
4 changes: 2 additions & 2 deletions java/client/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Contributor

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?

Copy link
Collaborator Author

@Yury-Fridlyand Yury-Fridlyand Oct 3, 2024

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.

Copy link
Contributor

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

testFfi.dependsOn('buildRustRelease')

test {
exclude "glide/ffi/FfiTest.class"
Expand Down
45 changes: 45 additions & 0 deletions java/client/src/main/java/glide/api/BaseClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
import static command_request.CommandRequestOuterClass.RequestType.ExpireTime;
import static command_request.CommandRequestOuterClass.RequestType.FCall;
import static command_request.CommandRequestOuterClass.RequestType.FCallReadOnly;
import static command_request.CommandRequestOuterClass.RequestType.FtCreate;
import static command_request.CommandRequestOuterClass.RequestType.FtDrop;
import static command_request.CommandRequestOuterClass.RequestType.FtSearch;
import static command_request.CommandRequestOuterClass.RequestType.GeoAdd;
import static command_request.CommandRequestOuterClass.RequestType.GeoDist;
import static command_request.CommandRequestOuterClass.RequestType.GeoHash;
Expand Down Expand Up @@ -209,6 +212,7 @@
import glide.api.commands.StreamBaseCommands;
import glide.api.commands.StringBaseCommands;
import glide.api.commands.TransactionsBaseCommands;
import glide.api.commands.VectorSearchBaseCommands;
import glide.api.models.ClusterValue;
import glide.api.models.GlideString;
import glide.api.models.PubSubMessage;
Expand Down Expand Up @@ -264,6 +268,9 @@
import glide.api.models.commands.stream.StreamReadGroupOptions;
import glide.api.models.commands.stream.StreamReadOptions;
import glide.api.models.commands.stream.StreamTrimOptions;
import glide.api.models.commands.vss.FTCreateOptions.FieldInfo;
import glide.api.models.commands.vss.FTCreateOptions.IndexType;
import glide.api.models.commands.vss.FTSearchOptions;
import glide.api.models.configuration.BaseClientConfiguration;
import glide.api.models.configuration.BaseSubscriptionConfiguration;
import glide.api.models.exceptions.ConfigurationError;
Expand All @@ -279,6 +286,7 @@
import glide.managers.CommandManager;
import glide.managers.ConnectionManager;
import glide.utils.ArgsBuilder;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.EnumSet;
import java.util.HashMap;
Expand Down Expand Up @@ -310,6 +318,7 @@ public abstract class BaseClient
HyperLogLogBaseCommands,
GeospatialIndicesBaseCommands,
ScriptingAndFunctionsBaseCommands,
VectorSearchBaseCommands,
TransactionsBaseCommands,
PubSubBaseCommands {

Expand Down Expand Up @@ -5142,4 +5151,40 @@ public CompletableFuture<Long> wait(long numreplicas, long timeout) {
new String[] {Long.toString(numreplicas), Long.toString(timeout)},
this::handleLongResponse);
}

@Override
public CompletableFuture<String> ftcreate(
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
@NonNull String indexName,
@NonNull IndexType indexType,
@NonNull String[] prefixes,
@NonNull FieldInfo[] fields) {
var args = new ArrayList<>(List.of(indexName, "ON", indexType.toString()));
if (prefixes.length > 0) {
args.add("PREFIX");
args.add(Integer.toString(prefixes.length));
args.addAll(List.of(prefixes));
}
args.add("SCHEMA");
Arrays.stream(fields).forEach(f -> args.addAll(List.of(f.toArgs())));

return commandManager.submitNewCommand(
FtCreate, args.toArray(String[]::new), this::handleStringResponse);
}

@Override
public CompletableFuture<Object[]> ftsearch(
@NonNull String indexName, @NonNull String query, @NonNull FTSearchOptions options) {
var args =
concatenateArrays(
new GlideString[] {gs(indexName), gs(query)},
options.toArgs(),
new GlideString[] {gs("DIALECT"), gs("2")});
Copy link
Collaborator

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.

return commandManager.submitNewCommand(FtSearch, args, this::handleArrayResponseBinary);
}

@Override
public CompletableFuture<String> ftdrop(@NonNull String indexName) {
return commandManager.submitNewCommand(
FtDrop, new String[] {indexName}, this::handleStringResponse);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/** Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0 */
package glide.api.commands;

import glide.api.models.commands.vss.FTCreateOptions.FieldInfo;
import glide.api.models.commands.vss.FTCreateOptions.IndexType;
import glide.api.models.commands.vss.FTSearchOptions;
import glide.api.models.commands.vss.FTSearchOptions.FTSearchOptionsBuilder;
import java.util.concurrent.CompletableFuture;

public interface VectorSearchBaseCommands {
Copy link
Contributor

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.

Copy link
Contributor

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();

Copy link
Collaborator Author

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

// TODO GlideString???
/**
* Creates an index and initiates a backfill of that index.
*
* @see TODO
* @param indexName The index name.
* @param indexType The index type.
* @param prefixes (Optional) A list of prefixes of index definitions
* @param fields Fields to populate into the index.
* @return <code>OK</code>.
* @example
* <pre>{@code
* // Create an index for vectors of size 2:
* client.ftcreate("hash_idx1", IndexType.HASH, new String[] {"hash:"}, new FieldInfo[] {
* new FieldInfo("vec", "VEC", VectorFieldFlat.builder(DistanceMetric.L2, 2).build())
* }).get();
* // Create a 6-dimensional JSON index using the HNSW algorithm:
* client.ftcreate("json_idx1", IndexType.JSON, new String[] {"json:"}, new FieldInfo[] {
* new FieldInfo("$.vec", "VEC", VectorFieldHnsw.builder(DistanceMetric.L2, 6).numberOfEdges(32).build())
* }).get();
* }</pre>
*/
CompletableFuture<String> ftcreate(
String indexName, IndexType indexType, String[] prefixes, FieldInfo[] fields);

/**
* Uses the provided query expression to locate keys within an index. Once located, the count
* and/or content of indexed fields within those keys can be returned.
*
* @see TODO
* @param indexName The index name to search into.
* @param query The text query to search.
* @param options The search options - see {@link FTSearchOptions}.
* @return A two element array, where first element is count of documents in result set, and the
* second element, which has format <code>
* {@literal Map<GlideString, Map<GlideString, GlideString>>}</code>, is a mapping between
* document names and map of their attributes.<br>
* If {@link FTSearchOptionsBuilder#count()} or {@link FTSearchOptionsBuilder#limit(int, int)}
* with values <code>0, 0</code> is set, the command returns array with only one element - the
* count of the documents.
* @example
* <pre>{@code
* byte[] vector = new byte[24];
* Arrays.fill(vector, (byte) 0);
* var result = client.ftsearch("json_idx1", "*=>[KNN 2 @VEC $query_vec]",
* FTSearchOptions.builder().params(Map.of("query_vec", gs(vector))).build())
* .get();
* assertArrayEquals(result, new Object[] { 2L, Map.of(
* gs("json:2"), Map.of(gs("__VEC_score"), gs("11.1100006104"), gs("$"), gs("{\"vec\":[1.1,1.2,1.3,1.4,1.5,1.6]}")),
* gs("json:0"), Map.of(gs("__VEC_score"), gs("91"), gs("$"), gs("{\"vec\":[1,2,3,4,5,6]}")))
* });
* }</pre>
*/
CompletableFuture<Object[]> ftsearch(String indexName, String query, FTSearchOptions options);

/**
* Deletes an index and associated content. Keys are unaffected.
*
* @see TODO
* @param indexName The index name.
* @return <code>OK</code>.
* @example
* <pre>{@code
* client.ftdrop("hash_idx1").get();
* }</pre>
*/
CompletableFuture<String> ftdrop(String indexName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
CompletableFuture<String> ftdrop(String indexName);
CompletableFuture<String> ftdropindex(String indexName);

}
Loading
Loading