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

change the semantics of TimeZoneDatabase from a product type to a sum type #213

Closed
BurntSushi opened this issue Jan 23, 2025 · 0 comments
Closed
Labels
breaking change Issues that require a breaking change for resolution. enhancement New feature or request

Comments

@BurntSushi
Copy link
Owner

Today, Jiff tries to load as many tzdb's as possible depending on the kind. For example, if for some reason Jiff can find a zoneinfo database, a concatenated database and has a bundled database, then it will look through all three on a tz lookup:

jiff/src/tz/db/mod.rs

Lines 358 to 388 in 94e8270

pub fn get(&self, name: &str) -> Result<TimeZone, Error> {
let inner = self.inner.as_deref().ok_or_else(|| {
if cfg!(feature = "std") {
err!(
"failed to find time zone `{name}` since there is no \
time zone database configured",
)
} else {
err!(
"failed to find time zone `{name}`, there is no \
global time zone database configured (and is currently \
impossible to do so without Jiff's `std` feature \
enabled, if you need this functionality, please file \
an issue on Jiff's tracker with your use case)",
)
}
})?;
if let Some(tz) = inner.zoneinfo.get(name) {
trace!("found time zone `{name}` in {:?}", inner.zoneinfo);
return Ok(tz);
}
if let Some(tz) = inner.concatenated.get(name) {
trace!("found time zone `{name}` in {:?}", inner.concatenated);
return Ok(tz);
}
if let Some(tz) = inner.bundled.get(name) {
trace!("found time zone `{name}` in {:?}", inner.bundled);
return Ok(tz);
}
Err(err!("failed to find time zone `{name}` in time zone database"))
}

I think I did things this way originally because I was thinking about a bundled database "augmenting" an incomplete zoneinfo database. But this is making less sense now with Jiff supporting Android's concatenated database. Overall, I think this is too much "smarts" for not a lot of gain. And it's somewhat less predictable in terms of its behavior.

In jiff 0.2, I plan to make TimeZoneDatabase represent only a single source of tzdb. And which tzdb is used depends on which is found. And different platforms can prioritize different tzdb sources. For example, on Android, I plan to prioritize the concatenated tzdb so that we aren't always looking for the zoneinfo tzdb unnecessarily.

@BurntSushi BurntSushi added breaking change Issues that require a breaking change for resolution. enhancement New feature or request labels Jan 23, 2025
BurntSushi added a commit that referenced this issue Jan 25, 2025
Jiff has three different kinds of databases that it can draw time zone
transitions from: the standard Unix zoneinfo database, a bundled or
embedded zoneinfo database (compiled into the binary), or the special
Android concatenated zoneinfo database. Previously, we would try to
create as many of these databases as possible, and then look all time
zones up in each.

I think that this type of semantic is very messy, because you can wind
up drawing from one db for one time zone and another db for another
(although in theory this shouldn't happen if they're all in sync). It
also requires that you always look for all three, which feels wrong.

Instead, we now just look for one and stop when we find it. Effectively,
we changed the internals from a product to a sum.

On Android, we check for a concatenated tzdb first, since that's likely
what we'll find.

Fixes #213
BurntSushi added a commit that referenced this issue Jan 25, 2025
Jiff has three different kinds of databases that it can draw time zone
transitions from: the standard Unix zoneinfo database, a bundled or
embedded zoneinfo database (compiled into the binary), or the special
Android concatenated zoneinfo database. Previously, we would try to
create as many of these databases as possible, and then look all time
zones up in each.

I think that this type of semantic is very messy, because you can wind
up drawing from one db for one time zone and another db for another
(although in theory this shouldn't happen if they're all in sync). It
also requires that you always look for all three, which feels wrong.

Instead, we now just look for one and stop when we find it. Effectively,
we changed the internals from a product to a sum.

On Android, we check for a concatenated tzdb first, since that's likely
what we'll find.

Fixes #213
BurntSushi added a commit that referenced this issue Jan 25, 2025
Jiff has three different kinds of databases that it can draw time zone
transitions from: the standard Unix zoneinfo database, a bundled or
embedded zoneinfo database (compiled into the binary), or the special
Android concatenated zoneinfo database. Previously, we would try to
create as many of these databases as possible, and then look all time
zones up in each.

I think that this type of semantic is very messy, because you can wind
up drawing from one db for one time zone and another db for another
(although in theory this shouldn't happen if they're all in sync). It
also requires that you always look for all three, which feels wrong.

Instead, we now just look for one and stop when we find it. Effectively,
we changed the internals from a product to a sum.

On Android, we check for a concatenated tzdb first, since that's likely
what we'll find.

Fixes #213
BurntSushi added a commit that referenced this issue Jan 25, 2025
Jiff has three different kinds of databases that it can draw time zone
transitions from: the standard Unix zoneinfo database, a bundled or
embedded zoneinfo database (compiled into the binary), or the special
Android concatenated zoneinfo database. Previously, we would try to
create as many of these databases as possible, and then look all time
zones up in each.

I think that this type of semantic is very messy, because you can wind
up drawing from one db for one time zone and another db for another
(although in theory this shouldn't happen if they're all in sync). It
also requires that you always look for all three, which feels wrong.

Instead, we now just look for one and stop when we find it. Effectively,
we changed the internals from a product to a sum.

On Android, we check for a concatenated tzdb first, since that's likely
what we'll find.

Fixes #213
BurntSushi added a commit that referenced this issue Jan 27, 2025
Jiff has three different kinds of databases that it can draw time zone
transitions from: the standard Unix zoneinfo database, a bundled or
embedded zoneinfo database (compiled into the binary), or the special
Android concatenated zoneinfo database. Previously, we would try to
create as many of these databases as possible, and then look all time
zones up in each.

I think that this type of semantic is very messy, because you can wind
up drawing from one db for one time zone and another db for another
(although in theory this shouldn't happen if they're all in sync). It
also requires that you always look for all three, which feels wrong.

Instead, we now just look for one and stop when we find it. Effectively,
we changed the internals from a product to a sum.

On Android, we check for a concatenated tzdb first, since that's likely
what we'll find.

Fixes #213
BurntSushi added a commit that referenced this issue Jan 30, 2025
Jiff has three different kinds of databases that it can draw time zone
transitions from: the standard Unix zoneinfo database, a bundled or
embedded zoneinfo database (compiled into the binary), or the special
Android concatenated zoneinfo database. Previously, we would try to
create as many of these databases as possible, and then look all time
zones up in each.

I think that this type of semantic is very messy, because you can wind
up drawing from one db for one time zone and another db for another
(although in theory this shouldn't happen if they're all in sync). It
also requires that you always look for all three, which feels wrong.

Instead, we now just look for one and stop when we find it. Effectively,
we changed the internals from a product to a sum.

On Android, we check for a concatenated tzdb first, since that's likely
what we'll find.

Fixes #213
BurntSushi added a commit that referenced this issue Feb 1, 2025
Jiff has three different kinds of databases that it can draw time zone
transitions from: the standard Unix zoneinfo database, a bundled or
embedded zoneinfo database (compiled into the binary), or the special
Android concatenated zoneinfo database. Previously, we would try to
create as many of these databases as possible, and then look all time
zones up in each.

I think that this type of semantic is very messy, because you can wind
up drawing from one db for one time zone and another db for another
(although in theory this shouldn't happen if they're all in sync). It
also requires that you always look for all three, which feels wrong.

Instead, we now just look for one and stop when we find it. Effectively,
we changed the internals from a product to a sum.

On Android, we check for a concatenated tzdb first, since that's likely
what we'll find.

Fixes #213
BurntSushi added a commit that referenced this issue Feb 2, 2025
Jiff has three different kinds of databases that it can draw time zone
transitions from: the standard Unix zoneinfo database, a bundled or
embedded zoneinfo database (compiled into the binary), or the special
Android concatenated zoneinfo database. Previously, we would try to
create as many of these databases as possible, and then look all time
zones up in each.

I think that this type of semantic is very messy, because you can wind
up drawing from one db for one time zone and another db for another
(although in theory this shouldn't happen if they're all in sync). It
also requires that you always look for all three, which feels wrong.

Instead, we now just look for one and stop when we find it. Effectively,
we changed the internals from a product to a sum.

On Android, we check for a concatenated tzdb first, since that's likely
what we'll find.

Fixes #213
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Issues that require a breaking change for resolution. enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant