watson-developer-cloud / swift-sdk

:iphone: The Watson Swift SDK enables developers to quickly add Watson Cognitive Computing services to their Swift applications.
https://watson-developer-cloud.github.io/swift-sdk/
Apache License 2.0
877 stars 222 forks source link

Add queries and query building #551

Closed schen22 closed 7 years ago

schen22 commented 7 years ago

Query building reference Query documentation

germanattanasio commented 7 years ago

@schen22 I don't want us to build a query builder in the sdk. Let's try to keep the models as simple as possible.

schen22 commented 7 years ago

@germanattanasio - Sure, that makes probably makes more sense. We could instead just reference the documentation and have users input their query as a string. How does that sound to you?

germanattanasio commented 7 years ago

There needs to be a middle point. We don't want users to write a lot of code but we don't also want to maintain a lot of custom code.

Can you come up with a class diagram? We can start from there.

On Mon, Dec 12, 2016 at 1:02 PM Sarah Chen notifications@github.com wrote:

@germanattanasio https://github.com/germanattanasio - Sure, that makes probably makes more sense. We could instead just reference the documentation and have users input their query as a string. How does that sound to you?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/watson-developer-cloud/ios-sdk/issues/551#issuecomment-266503806, or mute the thread https://github.com/notifications/unsubscribe-auth/AATHRRQv8zyjaroVdXGOVEk-y2boKnLyks5rHYwrgaJpZM4LKMpN .

schen22 commented 7 years ago

Here's a drafted class diagram for a query builder: https://drive.google.com/file/d/0B6_IZDgE6LbhZWpxbVhIRlBBQm8/view?usp=sharing

Across the board, Query, Filter and Aggregation models allow a user to enter an entire query expression. I was thinking of adding error handling methods, but this would take probably the same amount of time to define all the query parameters the Discovery service allows.

To look into the possibility of defining all the query parameters I specify the current methods and properties that the Discovery service currently allows the user to query for. However, this would require more maintenance in the future, as you mentioned.

A few questions that came up while I was mapping out the diagram:

kognate commented 7 years ago

As for the other sdks: they don't define the query response too tightly. The response is a json object. If you do aggregations, you'll get those back in the response too, and that can be pretty hairy. The query params are also dependent on the FQNs of the data ingested.

germanattanasio commented 7 years ago

Python and node don't need to parse the response like Swift or Java. It's a different world.

I think you went too deep with the request builder. The response needs to be parsed so I agree with that part of the diagram. I guess that matches 1-1 the JSON.

The way we build the query should be more high level. Just use the parameters you see in Swagger and make sure they are strings.

PLEASE, write code like if you were the user and see if you like what you see before start implementing this.

Let's actually move the conversation to be around the code devs will write. Can you write the Swift code that our devs will write to do a simple query with 1 aggregation?

kognate commented 7 years ago

I think if we could start with a playground and define some of this stuff it might be easier/better/kinder than using a class diagram? Especially since swift isn't especially OOP.

@germanattanasio swift can deal with JSON more cleanly than Java.

@schen22 I would argue an interface similar to NSPredicate would be preferable to the method style. Aggregations could be expressed and combined in a similar fashion. For example (see that attached playground). DIscoveryQuery.playground.zip

schen22 commented 7 years ago

@kognate - Looks good to me! Along similar lines, what do you think about implementing the protocol DiscoveryPredicateQueryString to have a public struct filterAggregation and similar structs for the rest of the aggregate methods?

DIscoveryQuery.playground 2.zip

kognate commented 7 years ago

my concern with filterAggregation is that it might be complicated to express something like this:

nested(entities).filter(entities.type:Person).term(entities.text.raw)

Filters also apply to results as well as aggregations.

DIscoveryQuery 3.zip

germanattanasio commented 7 years ago

In the java sdk we just expect strings. Again, I think we should keep it simple. On Tue, Dec 13, 2016 at 5:13 PM Joshua Smith notifications@github.com wrote:

my concern with filterAggregation is that it might be complicated to express something like this:

nested(entities).filter(entities.type:Person).term(entities.text.raw)

Filters also apply to results as well as aggregations.

DIscoveryQuery 3.zip https://github.com/watson-developer-cloud/ios-sdk/files/650331/DIscoveryQuery.3.zip

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/watson-developer-cloud/ios-sdk/issues/551#issuecomment-266878832, or mute the thread https://github.com/notifications/unsubscribe-auth/AATHRS3Hb7A8V74wZsXbk04-byca7db2ks5rHxiZgaJpZM4LKMpN .