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

api: specify inclusive min/max filters #200

Merged
merged 1 commit into from
Oct 25, 2022
Merged

Conversation

pro-wh
Copy link
Collaborator

@pro-wh pro-wh commented Oct 21, 2022

we had a lot of these min/max filters. the queries use >= <=, so I'm entering these as inclusive

actually the time ones are kinda not right to be called simply "inclusive." if I say the max time is 2022 oct 20, that doesn't mean a block etc from 2022 oct 20 at 9 PM is "included," right? it's only if the timestamp is exactly 2022 oct 20 00:00:00 and zero microseconds, then that's equal to the provided max time and that's included. but up to 999 nanoseconds is fine because that'll be lost when it gets converted from Go to postgresql. hm.

or am I wrong about the above?

@pro-wh pro-wh marked this pull request as ready for review October 21, 2022 22:35
@pro-wh pro-wh mentioned this pull request Oct 21, 2022
Base automatically changed from pro-wh/feature/emeraldapi to main October 21, 2022 22:36
@pro-wh pro-wh force-pushed the pro-wh/feature/openclose branch from e9e199c to 284f7ef Compare October 21, 2022 22:36
@mitjat
Copy link
Contributor

mitjat commented Oct 24, 2022

That's my understanding of how times work too, yes. Tricky to describe the behavior correctly and succinctly ...

WDYT about changing the queries to be inclusive on the lower limit and exclusive on the upper? (i.e. >= and <)
That would make times behave naturally. And it would give us an API that is (in my experience) in line with most other range-related APIs (e.g. python slices). It also would make it easier to "scan" a range with multiple queries (e.g. fetching tx with fees from the interval [A,B), then from [B,C), etc; not that I have a great use case for that :).

@aefhm
Copy link
Contributor

aefhm commented Oct 24, 2022

I like the scanning idea.

actually the time ones are kinda not right to be called simply "inclusive." if I say the max time is 2022 oct 20, that doesn't mean a block etc from 2022 oct 20 at 9 PM is "included," right?

I think the inclusive search is more "expected?" People normally think of it as an UTC timestamp comparison?

Copy link
Contributor

@aefhm aefhm left a comment

Choose a reason for hiding this comment

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

I'm cool with the description change.

@pro-wh
Copy link
Collaborator Author

pro-wh commented Oct 24, 2022

interesting, I think the pagination idea from earlier wanted that too. although it would go backwards: [-inf, inf) limit 20, then you look at the earliest one B and request [-inf, B) limit 20, etc.

@pro-wh
Copy link
Collaborator Author

pro-wh commented Oct 25, 2022

filed 203. let's merge this

@pro-wh pro-wh merged commit f4189bd into main Oct 25, 2022
@pro-wh pro-wh deleted the pro-wh/feature/openclose branch October 25, 2022 21:56
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.

3 participants