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: add FT.INFO #2405

Merged
merged 3 commits into from
Oct 18, 2024
Merged

Java: add FT.INFO #2405

merged 3 commits into from
Oct 18, 2024

Conversation

Yury-Fridlyand
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand commented Oct 8, 2024

Issue link

#2428

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Commits will be squashed upon merging.

@Yury-Fridlyand Yury-Fridlyand added the java issues and fixes related to the java client label Oct 8, 2024
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner October 8, 2024 23:05
@Yury-Fridlyand Yury-Fridlyand force-pushed the java/yuryf-ft-info-valkey-423 branch from 9767901 to 27886e5 Compare October 11, 2024 19:27
@Yury-Fridlyand Yury-Fridlyand changed the base branch from main to release-1.2 October 11, 2024 19:28
@Yury-Fridlyand Yury-Fridlyand force-pushed the java/yuryf-ft-info-valkey-423 branch from 27886e5 to a23acfa Compare October 11, 2024 19:29
@Yury-Fridlyand Yury-Fridlyand added this to the 1.2 milestone Oct 11, 2024
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Oct 11, 2024
13 tasks
Comment on lines 961 to 1100
for pair in map.iter_mut() {
if pair.0 == Value::SimpleString("fields".into()) {
let Value::Array(mut fields) = pair.1.clone() else {
return Err((
ErrorKind::TypeError,
"Response couldn't be converted for FT.INFO",
format!("(`fields` was {:?})", get_value_type(&pair.1.clone())),
)
.into());
};

for field in fields.iter_mut() {
let Value::Map(mut field_params) = convert_to_expected_type(field.clone(), Some(ExpectedReturnType::Map {
key_type: &None,
value_type: &None,
})).unwrap() else { unreachable!() };

for pair in field_params.iter_mut() {
if pair.0 == Value::SimpleString("vector_params".into()) {
*pair = (pair.0.clone(), convert_to_expected_type(pair.1.clone(), Some(ExpectedReturnType::Map {
key_type: &None,
value_type: &None,
}))?);
break;
}
}

*field = Value::Map(field_params);
}

*pair = (pair.0.clone(), Value::Array(fields));
break;
}
}
Ok(Value::Map(map))
Copy link
Collaborator

@jonathanl-bq jonathanl-bq Oct 14, 2024

Choose a reason for hiding this comment

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

As requested, I dug into this a bit more to see if I could reduce the number of clones here. Here's what I've come up with:

Suggested change
for pair in map.iter_mut() {
if pair.0 == Value::SimpleString("fields".into()) {
let Value::Array(mut fields) = pair.1.clone() else {
return Err((
ErrorKind::TypeError,
"Response couldn't be converted for FT.INFO",
format!("(`fields` was {:?})", get_value_type(&pair.1.clone())),
)
.into());
};
for field in fields.iter_mut() {
let Value::Map(mut field_params) = convert_to_expected_type(field.clone(), Some(ExpectedReturnType::Map {
key_type: &None,
value_type: &None,
})).unwrap() else { unreachable!() };
for pair in field_params.iter_mut() {
if pair.0 == Value::SimpleString("vector_params".into()) {
*pair = (pair.0.clone(), convert_to_expected_type(pair.1.clone(), Some(ExpectedReturnType::Map {
key_type: &None,
value_type: &None,
}))?);
break;
}
}
*field = Value::Map(field_params);
}
*pair = (pair.0.clone(), Value::Array(fields));
break;
}
}
Ok(Value::Map(map))
let Some(fields_pair) = map.iter_mut().find(|(key, _)| {
*key == Value::SimpleString("fields".into())
}) else { return Ok(Value::Map(map)) };
let (fields_key, fields_value) = std::mem::replace(fields_pair, (Value::Nil, Value::Nil));
let Value::Array(fields) = fields_value else {
return Err((
ErrorKind::TypeError,
"Response couldn't be converted for FT.INFO",
format!("(`fields` was {:?})", get_value_type(&fields_value)),
).into());
};
let fields = fields.into_iter().map(|field| {
let Value::Map(mut field_params) = convert_to_expected_type(field, Some(ExpectedReturnType::Map {
key_type: &None,
value_type: &None,
}))? else { unreachable!() };
let Some(vector_params_pair) = field_params.iter_mut().find(|(key, _)| {
*key == Value::SimpleString("vector_params".into())
}) else { return Ok(Value::Map(field_params)) };
let (vector_params_key, vector_params_value) = std::mem::replace(vector_params_pair, (Value::Nil, Value::Nil));
let _ = std::mem::replace(vector_params_pair, (vector_params_key, convert_to_expected_type(vector_params_value, Some(ExpectedReturnType::Map {
key_type: &None,
value_type: &None,
}))?));
Ok(Value::Map(field_params))
}).collect::<RedisResult<Vec<Value>>>()?;
let _ = std::mem::replace(fields_pair, (fields_key, Value::Array(fields)));
Ok(Value::Map(map))

I gathered some stats to show that the number of allocations is reduced from 36 to 23. You might be wondering if that collect call allocates memory. I checked, and it does not. The number of allocations is still 23, even if you use a mem replace instead.

Here's what heaptrack produces when I run this code in a small sample app:

Before
image

After
image

Here is the sample app code (I expose glide_core client types and functions in public API just for testing purposes):

use redis::Value;
use glide_core::client::{ExpectedReturnType, convert_to_expected_type};

fn main() {
    let value = Value::Array(vec![
        Value::Array(vec![Value::SimpleString("fields".to_string()), Value::Array(vec![
            Value::Array(vec![
                Value::SimpleString("vector_params".to_string()),
                Value::Array(vec![Value::SimpleString("hi".to_string()), Value::Int(42)])
            ])
        ])])
    ]);
    convert_to_expected_type(value, Some(ExpectedReturnType::FTInfoReturnType)).unwrap();
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated, thank you for the review!

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand Yury-Fridlyand force-pushed the java/yuryf-ft-info-valkey-423 branch from 4274625 to 5a4cba3 Compare October 18, 2024 19:24
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand Yury-Fridlyand merged commit d291d87 into release-1.2 Oct 18, 2024
34 of 36 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the java/yuryf-ft-info-valkey-423 branch October 18, 2024 20:01
avifenesh pushed a commit to avifenesh/valkey-glide that referenced this pull request Oct 21, 2024
* `FT.INFO`

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
avifenesh pushed a commit to avifenesh/valkey-glide that referenced this pull request Oct 21, 2024
* `FT.INFO`

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Muhammad-awawdi-amazon pushed a commit to Muhammad-awawdi-amazon/valkey-glide that referenced this pull request Oct 22, 2024
* `FT.INFO`

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java issues and fixes related to the java client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants