-
Notifications
You must be signed in to change notification settings - Fork 22
feat: Add experimental count relationships by filter to Materialize API #148
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
feat: Add experimental count relationships by filter to Materialize API #148
Conversation
ea11803 to
f5fb9ba
Compare
f5fb9ba to
9eb6e4a
Compare
|
|
||
| message ExperimentalCountRelationshipsByFilterResponse { | ||
| // relationship_count is the count of relationships that match the filter. | ||
| uint64 relationship_count = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this not have the required tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally don't mark required on response types and basically forgot to do so; it will auto-fill to zero anyway as an int, so marking it required doesn't really make sense
| } | ||
| } | ||
| }, | ||
| "ExperimentalCountRelationshipsByFilterResponse": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This swagger looks odd; is it including ALL the Materialize stuff? it doesn't look like it
why did it include this response object and not the API itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh it's because of this line
Line 10 in 3454618
| - "disable_service_tags=true" # Hide the Materialize APIs, which aren't exposed via HTTP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, although it doesn't really matter either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds a new experimental count relationships API that can be run on the Materialize instance, to avoid performing the count on the main datastore