Kitware / minerva

Minerva: client/server/services for analysis and visualization
Apache License 2.0
36 stars 14 forks source link

Postgres builder preview #489

Closed matthewma7 closed 6 years ago

matthewma7 commented 6 years ago

The PR add a feature to see query result metadata.

The first commit refactor the existing code to make it more generic across file geojson, postgres geojson, and timeseries-geosjon datasets. This changed involved a few moving parts but they reconciled now.

The other two commit implement a server-side endpoint for getting the result metadata and the client GUI. Due to performance consideration, the result metadata is retrieved on request. Also, it requests the user to request the result metadata before creating the dataset to catch query issue early.

2018-02-28_10-31-36

===Updated=== chrome_2018-03-06_11-04-13

aashish24 commented 6 years ago

@matthewma7 this looks great. I am wondering if Metadata should be a checkbox instead of a button? As I would think that would be more useful or else users would have to do it all the time. Also, I am wondering if parentheses are necessary.

matthewma7 commented 6 years ago

@aashish24 I see. Now I look at it. The parenthesis looks unnecessary. The information in the parenthesis is actually important as well.

My first implementation was trying to update the metadata result automatically as the user build the query, but later, I found I ran into performance problem on the server and race condition on the client (which is, of course, fixable). Then I start to think because this is an open-ended query building, some high-cost query could be easily generated, so a manual triggering will be safer (I actually give a warning if the condition could be costly, such as grouping on the geometry field without filtering, which runs around 30 second for me). In the GIF, I showed different condition so I clicked it multiple times, but I think I see this as a safety net instead of an interactive tool for the user to learn about the data, I envision a user who knows the data would click the button a few times at most. Hope this explains this explicit manual design.

aashish24 commented 6 years ago

@aashish24 I see. Now I look at it. The parenthesis looks unnecessary. The information in the parenthesis is actually important as well.

+1, sounds like we agree that we can remove them.

My first implementation was trying to update the metadata result automatically as the user build the query, but later, I found I ran into performance problem on the server and race condition on the client (which is, of course, fixable). Then I start to think because this is an open-ended query building, some high-cost query could be easily generated, so a manual triggering will be safer (I actually give a warning if the condition could be costly, such as grouping on the geometry field without filtering, which runs around 30 second for me). In the GIF, I showed different condition so I clicked it multiple times, but I think I see this as a safety net instead of an interactive tool for the user to learn about the data, I envision a user who knows the data would click the button a few times at most. Hope this explains this explicit manual design.

makes sense. Should we call the button "Query Data" "Metadata" reads confusing to me as a domain scientist but perhaps it is better. Just a though, I do not have strong say.

dorukozturk commented 6 years ago

I agree. Maybe "Inspect Query" or something in that nature might be better than "Metadata".

matthewma7 commented 6 years ago

@dorukozturk @aashish24 Thanks! I think Inspect would work well, it works well with the magnifier too. Metadata is harder to grasp.

aashish24 commented 6 years ago

I agree. Maybe "Inspect Query" or something in that nature might be better than "Metadata".

+1 for Inspect nice suggestion @dorukozturk

matthewma7 commented 6 years ago

@dorukozturk, @aashish24 Thanks for your review.