sea-query icon indicating copy to clipboard operation
sea-query copied to clipboard

Streamline quoting

Open Expurple opened this issue 1 month ago • 1 comments

I was very suprised and confused when I noticed the following in the codebase:

  • A comment on Iden::quoted, saying that it doesn't actually return a quoted string (!). In fact, the quoting is done later by the (DB-specific) backend.
  • fn is_static_iden that's not actually about knowing the identifier at compile time. In fact, it's about having special characters in the identifier (and the need to escape the identifier in the backend).
  • The expensive use of Cow::Owned to indicate the presense of special characters (rather than to simply store a runtime-provided string).

It's so weird and non-obvious.

Here, I streamline things:

  • Removed the Iden::quoted method that isn't actually quoted.

    I understand that this is a breaking change. But if the users can't actually get a properly-quoted string without a specific DB backend, then we should help them notice this reality.

  • Made Iden::unquoted return a Cow, as that's the method that actually constructs a DynIden (Cow).

    This is a breaking change.

  • Replaced IdenStatic: Iden with impl<T> Iden for T where T: IdenStatic.

    Technically a breaking change for manual implementors, but most people probably use derives.

  • Removed all other extra supertraits from IdenStatic. All tests pass without those, and it's not clear why those were needed. Those are probably leftowers from the old Iden system. If those are actually needed, we should test and document that.

    Technically a breaking change, although I doubt that many people worked with generic IdenStatic and relied on those implied bounds.

  • Replaced impl Iden with impl IdenStatic everywhere where the string is static (e.g., in all derives).

  • Removed unnecessary allocations of Cow::Owned.

  • Removed is_static_iden.

    This isn't a breaking change because we didn't have that in 1.1.x.

  • Updated the docs.

To be fair, I understand the intended meaning and benefit of is_static_iden and indicating it with Cow::Borrowed. When we know at compile time that the identifier doesn't have any special characters, the backend can write_str it as-is, in one go. Because that's no longer the case, my PR may regress the performance a little.

However:

  • The correctness of that approach is hard to verify. I'm not sure that we construct Cow::Borrowed only when the string doesn't have special characters. It's easy to choose Cow::Borrowed simply because you have a static string (whether or not it contains special characters). That's the most obvious thing to do.
  • My PR wins back performance by avoiding allocations and using Cow::Borrowed more often.
  • If we really want to keep a fast write_str path in the backend, we can put the is_static_iden check right there before the write. But I wouldn't worry about that.

The main thing that I'm going for here is conceptual clarity, rather than performance:

  • All static identifiers now implement IdenStatic.
  • No more qouted that isn't actually quoted.
  • No more valid static identifiers that return false from is_static_iden.

Expurple avatar Dec 07 '25 13:12 Expurple