ibis icon indicating copy to clipboard operation
ibis copied to clipboard

feat(polars): add more accurate type mapping for timestamps

Open ncclementi opened this issue 2 years ago • 2 comments

  • Closes https://github.com/ibis-project/ibis/issues/8479

polars datetime types have a fixed time unit (us, ns, or ms). Ibis timestamp types do too (but we also have seocnds as an option). In the case of polars we're currently mapping all timestamps to ns, this PR makes things a bit more specific and uses proper units.

I'm still not sure how to handle the case Ibis seconds to maybe ms in polars ? Need a bit help on how the conversion of units happens, more specifically where is the value part handled? we would need to do ibis_seconds*1000 = polars_ms at the moment I'm raising, similarly to what we do in durations, but I need to tackle this differently, see all the current failures in CI.

ncclementi avatar Apr 12 '24 16:04 ncclementi

After looking more into this PR, I'm not sure if we are correctly casting the values when mapping the timestamps, base on this code:

https://github.com/ibis-project/ibis/blob/b48f451de77bd9af832cea10d7ddf8c44097c45a/ibis/backends/polars/compiler.py#L157-L171

That being said the casting "potential issue" is separate from the mapping of the types, so I proposed opening a separate issue to investigate that, and for now get this PR in.

ncclementi avatar Apr 26 '24 17:04 ncclementi

I did a little more digging and on main (as well as this branch). The casting is broken, see https://github.com/ibis-project/ibis/issues/9091

I'm not sure what are the next steps here, regarding this PR.

ncclementi avatar Apr 30 '24 20:04 ncclementi

@ncclementi Anything blocking this PR? I know it's been awhile, but does it make sense to merge this in for 9.2?

cpcloud avatar Jul 16 '24 15:07 cpcloud

Technically no, as the typing gets fixed in this PR, although the actual casting is kind of broken see https://github.com/ibis-project/ibis/issues/9091.

Let me rebase, and kick CI again. I'd be ok merging this and we can look into the casting of the values in a followup.

ncclementi avatar Jul 16 '24 16:07 ncclementi