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

Implement Near Search Operator #62

Merged
merged 8 commits into from
Oct 3, 2023

Conversation

username1103
Copy link
Contributor

resolve #13


Show

Database annotations

The Near Operator example uses two databases in mongodb documetation. Therefore, it is necessary to inject two MongoTemplates, so I implemented it by adding the annotation Database. Please give your opinion on this.

pivot verification

Additionally, the document specifies that Near's pivot must be greater than 0. I'm wondering whether to add this as validation. What do you think?

Path type settings

Origin can include Number, ISODate, and GeoJsonPoint. So path has to be name of Number, ISODate, GeoJsonPoint fields. I attempted to override methods for each type, but encountered problem due to generics not being recognized differently. So an annotation called JvmName was added.

- add Database Annotation
Near Example use multi database. So need multi mongo template.
@username1103 username1103 changed the title implement Near Search Operator Implement Near Search Operator Oct 2, 2023
Comment on lines +103 to +105
* pivot
* score = ------------------
* pivot + distance
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 better if these lines are wrapped in code block.

Suggested change
* pivot
* score = ------------------
* pivot + distance
*
* ```
* pivot
* score = ------------------
* pivot + distance
* ```
*

* Origin to query for Number field.
* This is the origin from which the proximity of the results is measured.
*
* For number fields, the value must be of BSON int32, int64, or double data types.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is missing @param docs for origin and pivot method :)

@Repository
class NearSearchRepository(
@Database("sample_mflix") private val mflixMongoTemplate: MongoTemplate,
@Database("sample_airbnb") private val airbnbMongoTamplate: MongoTemplate,
Copy link
Contributor

Choose a reason for hiding this comment

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

There is some typo. :)

Suggested change
@Database("sample_airbnb") private val airbnbMongoTamplate: MongoTemplate,
@Database("sample_airbnb") private val airbnbMongoTemplate: MongoTemplate,

val score: Double,
)

fun findByRuntime(): AggregationResults<RuntimeDto> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be better to add @see docs to each methods :)

Suggested change
fun findByRuntime(): AggregationResults<RuntimeDto> {
/**
* @see <a href="https://www.mongodb.com/docs/atlas/atlas-search/near/#number-example">Number Example</a>
*/
fun findByRuntime(): AggregationResults<RuntimeDto> {

search {
index = "runtimes"
near {
path("runtime")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have collection of movie document,
We can use KProperty instead of String.
I think it would be better to guide users to use KProperty instead of strings if possible.
(it applies project stage too)

Suggested change
path("runtime")
path(Movies::runtime)

}
}

return mflixMongoTemplate.aggregate(aggregation, "movies", RuntimeDto::class.java)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use extension function like this :)

Suggested change
return mflixMongoTemplate.aggregate(aggregation, "movies", RuntimeDto::class.java)
return mflixMongoTemplate.aggregate<Movies, RuntimeDto >(aggregation)

Comment on lines 54 to 57
val database = repositoryParameter.annotations.find { it is Database } as Database?
if (database != null) {
return@associateWith getMongoTemplate(getConnectionString(database.value))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To remove warning from return@associateWith.
we can use let scope function.

Suggested change
val database = repositoryParameter.annotations.find { it is Database } as Database?
if (database != null) {
return@associateWith getMongoTemplate(getConnectionString(database.value))
}
repositoryParameter.findAnnotation<Database>()?.let {
getMongoTemplate(getConnectionString(it.value))
} ?: mongoTemplate

val connectionString = "mongodb+srv://$ATLAS_DOMAIN/${atlasTest.database}?retryWrites=true&w=majority"
val mongoTemplate = MongoTemplate(SimpleMongoClientDatabaseFactory(connectionString))
val connectionString = getConnectionString(atlasTest.database)
val mongoTemplate = getMongoTemplate(connectionString)

val parameters = testConstructor.parameters.associateWith { parameter ->
val parameterClass = parameter.type.classifier as KClass<*>
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don mind, Could you change 25, 43 lines to use findAnnotation extension function?

      val atlasTest = clazz.findAnnotation<AtlasTest>() ?: return null

     checkNotNull(parameterClass.findAnnotation<Repository>()) {

@jbl428
Copy link
Contributor

jbl428 commented Oct 2, 2023

Database annotations
The Near Operator example uses two databases in mongodb documetation. Therefore, it is necessary to inject two MongoTemplates, so I implemented it by adding the annotation Database. Please give your opinion on this.

I think your idea is great 👍

pivot verification
Additionally, the document specifies that Near's pivot must be greater than 0. I'm wondering whether to add this as validation. What do you think?

We can certainly add the validation you mentioned, but currently, we are not add value validation in the project.
Because this project prioritizes providing a type-safe DSL rather than value validation.

Path type settings
Origin can include Number, ISODate, and GeoJsonPoint. So path has to be name of Number, ISODate, GeoJsonPoint fields. I attempted to override methods for each type, but encountered problem due to generics not being recognized differently. So an annotation called JvmName was added.

You are right. there are some operators that @JvmName annotation is applied too.

@hong3-inflab
Copy link
Member

@jbl428 Thanks, I've corrected the things you mentioned.

Copy link
Contributor

@jbl428 jbl428 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jbl428 jbl428 merged commit bcc4231 into inflearn:main Oct 3, 2023
1 check passed
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.

Implement NearSearchOperatorDsl
3 participants