Allow non-scalar values as tagged union keys

[adriangb]

See also pydantic/pydantic#5958

Selected Reviewer: @samuelcolvin

[codspeed-hq]

CodSpeed Performance Report

Merging #655 tagged-union-enum (83c2ceb) will not alter performances.

Summary

🔥 4 improvements
❌ 0 regressions
✅ 121 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

Benchmarks breakdown

	Benchmark	main	tagged-union-enum	Change
🔥	test_validate_literal[json-few_small_strings]	26.8 µs	20.7 µs	22.81%
🔥	test_validate_literal[json-few_large_strings]	28.2 µs	22 µs	22.14%
🔥	test_validate_literal[json-many_small_strings]	43.1 µs	20.9 µs	51.40%
🔥	test_validate_literal[json-many_large_strings]	28.7 µs	22.2 µs	22.87%

[adriangb]

please review

[codecov-commenter]

Codecov Report

Merging #655 (83c2ceb) into main (d2f4226) will decrease coverage by 0.16%.
The diff coverage is 89.90%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #655      +/-   ##
==========================================
- Coverage   93.90%   93.74%   -0.16%     
==========================================
  Files          99       99              
  Lines       13756    13726      -30     
  Branches       25       25              
==========================================
- Hits        12917    12868      -49     
- Misses        833      852      +19     
  Partials        6        6              

Impacted Files	Coverage Δ
src/lookup_key.rs	92.38% <ø> (ø)
src/validators/union.rs	81.48% <64.51%> (-5.67%)	⬇️
pydantic_core/core_schema.py	96.88% <100.00%> (ø)
src/input/input_abstract.rs	86.11% <100.00%> (+0.81%)	⬆️
src/input/input_python.rs	98.06% <100.00%> (-0.26%)	⬇️
src/validators/literal.rs	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2f4226...83c2ceb. Read the comment docs.

[dmontagu]

Does this solve the enum discriminator thing on the pydantic side? Also, are there any (non-benchmarked?) (negative) performance implications? Not that that would make me reject it but I think it's worth understanding.

[adriangb]

Does this solve the enum discriminator thing on the pydantic side?
It does, although I had forgotten to remove the xfail. It doesn't allow validating scalar values against a non-scalar Enum even if it has scalar values (see pydantic/pydantic@cc6577d#diff-3e4f2ce6ba438060e163151503e9bb6cea70760b0465f1a6e17a8646826a9b7d).

Also, are there any (non-benchmarked?) (negative) performance implications? Not that that would make me reject it but I think it's worth understanding.

It doesn't seem like we have any benchmarks. I can add some to main and rebase this PR.

[adriangb]

Okay so small performance regression for JSON, probably because we now convert to Python just to match. We could do the same thing we do with literal validator where we try to match a Rust string/int before using a Python dict. The downside of that (aside from adding complexity) is that the match order is no longer preserved w.r.t. the input schema.

[samuelcolvin]

Without looking through this in line-by-line detail, this looks good to me.

[samuelcolvin]

In general I think this looks great.

I want to be a bit more sure that the performance regressions are an artefact of codspeed, not real - maybe we could create another trivial PR and see if we get the same thing?

@art049 is there any chance this big change in benchmarks is a result of issues with Codspeed?

[samuelcolvin]

please update.

[art049]

@samuelcolvin @adriangb I'm having a look to understand if it is noise or not since apparently it also happened in #656 . I'll keep you updated.

Edit: let's follow up on #656 since it's easier to understand the behavior there.

[adriangb]

please review

[adriangb]

There we go now this has no performance degradations and actually makes some things 50% faster @samuelcolvin

[adriangb]

Since I think all of the feedback (including performance regressions) has been addressed I'm going to go ahead and merge this