Skip to content

Conversation

@ruchirK
Copy link
Contributor

@ruchirK ruchirK commented Nov 17, 2025

This commit adds two small changes to improve how we parse TTLMapConfigs.

  1. It strictly enforces that tick is an integer multiple of ttl. Previously, we only recommened that in comments, with the consequence that:
true_ttl = desired_ttl - (desired_ttl % tick) <= desired_ttl

In the worst case, if tick is close to ttl / 2 then the true ttl would be equivalent to tick. For example, if the requested ttl was 60 seconds, and the tick was 31 seconds, we would only allocate a single list of inputs to delete and free them every tick.

  1. There was a small bug in the validation logic that did not correctly validate that tick was nonzero due to an incorrect application of DeMorgan's law. This commit fixes that to correctly check that tick is nonzero.

Copy link
Collaborator

@jayshrivastava jayshrivastava left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for doing this! Let's add a test case for this too.

This commit adds two small changes to improve how we parse TTLMapConfigs.

1. It strictly enforces that `tick` is an integer multiple of `ttl`. Previously,
we only recommened that in comments, with the consequence that:

```
true_ttl = desired_ttl - (desired_ttl % tick) <= desired_ttl
```
In the worst case, if `tick` is close to `ttl / 2` then the true `ttl` would
be equivalent to `tick`. For example, if the requested `ttl` was 60 seconds, and the tick
was 31 seconds, we would only allocate a single list of inputs to delete and free them every
tick.

2. There was a small bug in the validation logic that did not correctly validate that tick
was nonzero due to an incorrect application of DeMorgan's law. This commit fixes that to
correctly check that `tick` is nonzero.
@ruchirK ruchirK force-pushed the fix-ttl-tick-multiple branch from d647623 to ee94bb4 Compare November 17, 2025 19:39
@ruchirK
Copy link
Contributor Author

ruchirK commented Nov 17, 2025

Looks good. Thanks for doing this! Let's add a test case for this too.

Done + also changed is_valid to no longer redundantly take args

Copy link
Collaborator

@gabotechs gabotechs left a comment

Choose a reason for hiding this comment

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

Nice!

@gabotechs gabotechs merged commit 8a2428b into datafusion-contrib:main Nov 18, 2025
4 checks passed
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