perf: Use direct path for time/timedelta literals

[mcrumiller]

Slight followup to #16018, allows direct construction of literals from datetime.time and datetime.timedelta objects.

[mcrumiller]

Failures are deltalake, unrelated to PR.

[alexander-beedie]

I don't think the added tests actually cover the change 🤔 The existing Python pl.lit already handles both types, your update enhances the lower-level Rust fn lit (definitely worthwhile), but the tests look to be validating the existing functionality?

[mcrumiller]

I don't think the added tests actually cover the change 🤔 The existing Python pl.lit already handles both types, your update enhances the lower-level Rust fn lit (definitely worthwhile), but the tests look to be validating the existing functionality?

lit.py was updated to pass the time and timedelta values directly to plr.lit, as opposed to the prior implementation that first converted their values to int. Does this not test the new functionality?

[codspeed-hq]

CodSpeed Performance Report

Merging #18223 will not alter performance

_{Comparing mcrumiller:literal-time (e622fd9) with main (1dc2533)}

Summary

✅ 37 untouched benchmarks

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.22%. Comparing base (7654387) to head (e622fd9).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #18223      +/-   ##
==========================================
- Coverage   80.30%   80.22%   -0.08%     
==========================================
  Files        1499     1500       +1     
  Lines      198744   198886     +142     
  Branches     2837     2837              
==========================================
- Hits       159604   159561      -43     
- Misses      38613    38798     +185     
  Partials      527      527              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mcrumiller]

My failed test is an apparently rare hypothesis case; I'll open an issue, maybe @MarcoGorelli can take a look.

Update: #18239.

[ritchie46]

Thanks. I think it is cleaner than our previous cast solution. Can you rebase, as we just moved the whole logic to a new crate, I doubt we can merge as is.

And there is a rogue print.

[ritchie46]

rogue print

[mcrumiller]

@ritchie46 I had rebased earlier. Just removed the print. Can you or @alexander-beedie verify that his concern is actually covered here?

[ritchie46]

These tests can be better. You should call explain(optimized=False) and check there is no cast in there.

[ritchie46]

@ritchie46 I had rebased earlier. Just removed the print. Can you or @alexander-beedie verify that his concern is actually covered here?

See comment. The test would already pass without this PR. My suggestion would be better.

[mcrumiller]

@ritchie46 I added the check, but the test still passes on the main branch anyway. Any further suggestions? I think the fact that the test still works despite the PR should be enough, no?

[ritchie46]

Yes, the IR conversion optimizes it away. So the test is not super useful. Let's remove the test and then merge. :P

[mcrumiller]

@ritchie46 removed the cast check. The lit tests are still worthwhile to have to ensure we can still create literals correctly.