Skip to content

[feature-search-json] Review before merge to master #349

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

Merged
merged 45 commits into from
May 24, 2021

Conversation

rafie
Copy link
Contributor

@rafie rafie commented May 5, 2021

No description provided.

rafie and others added 22 commits March 27, 2021 18:03
* WIP Add API openKeyFromStr

* WIP Align ordering of struct RedisJSONAPI_V1 in C and Rust

* WIP return bulk string

* WIP fix bulk string

* WIP Add APIs "FromKey" and tuning
…API_openKey" (#300)

This reverts commit 57f10c2
Fix is done in better way by commit 036cb51 in redismodule-rs
"WIP open_with_redis_string does not take ownership of the RedisModuleString"
[feature-search-json] Sync with master (1)
* Update dependency redismodule-rs to be from master and fix warnings

* Fix Formatting
* Update to Redis 6.2.2 (#332)

* Fix json.arrappend performance (#324)

* improve arr_pop, arr_insert & arr_trim performance 
* refactor do_json_* functions

* normalize index for backward (#327)

* normalize index for backward
* adpat for full backward with 1.0.x

* fix #318 (#333)

* CircleCI: macOS and benchmark fixes (#337)

* Update config.yml (#338)

* Add JSON.CLEAR command (#306)

* Restore optional path (default is $) for JSON.DEL

* Add JSON.CLEAR

* Handle JSON.CLEAR with $ path

* Add JSON.CLEAR - Fix failure in test:testSetInvalidPathShouldFail

* Idiomatic fixes per Gavrie review

Co-authored-by: Guy Korland <gkorland@gmail.com>

* Update RS_VERSIONS

last promoted builds

* Document flags for running tests

* fix arr_pop backward to 1.0 (#345)

* JSON.INSERTARR backward compatability for 1.0 (#343)

* Fix JSON.INSERTARR backward compatibility with 1.0.x
* Update test.py

* Fix build error
(use of undeclared crate or module `rawmod`)

* Fix format

Co-authored-by: Guy Korland <gkorland@gmail.com>
Co-authored-by: Omer Shadmi <76992134+oshadmi@users.noreply.github.com>
Co-authored-by: tomerhekredis <72793005+tomerhekredis@users.noreply.github.com>
Co-authored-by: Gavrie Philipson <gavrie@philipson.co.il>
Co-authored-by: oshadmi <omer.shadmi@redislabs.com>
src/c_api.rs Outdated
Comment on lines 312 to 319
Value::Bool(ref b) => return if *b { Some(1) } else { Some(0) },
Value::Number(n) => {
if let Some(i) = n.as_i64() {
return if i != 0 { Some(1) } else { Some(0) };
} else if let Some(f) = n.as_f64() {
return if f != 0.0 { Some(1) } else { Some(0) };
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified as above.

//---------------------------------------------------------------------------------------------

pub struct JSONApiKey<'a> {
_key: RedisKeyWritable,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the _key field necessary if it's unused?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is needed for life-cycle management:
It will be dropped (and inner key will be closed) when JSONAPI_closeKey is called.
No need to use this field for other purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to document this in a comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, will add a comment

gavrie
gavrie previously approved these changes May 11, 2021
Fix per Guy's review - aux load and save
gkorland
gkorland previously approved these changes May 19, 2021
@oshadmi oshadmi requested a review from gavrie May 20, 2021 11:50
oshadmi and others added 2 commits May 23, 2021 14:24
* [skip ci] WIP - JSON CAPI - add isJSON

* JSON CAPI - add isJSON (currently depend on redismodule-rs git branch)

* code style change

Co-authored-by: Guy Korland <gkorland@gmail.com>

* Fix format

* Bump Dependency to redis-module crate version 0.17.0

Co-authored-by: Guy Korland <gkorland@gmail.com>
Copy link
Collaborator

@oshadmi oshadmi left a comment

Choose a reason for hiding this comment

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

👏🏼 👍🏼

@rafie rafie merged commit 5d07ce5 into master May 24, 2021
@rafie rafie deleted the rafi-sj-merge-to-master branch May 24, 2021 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants